Bug 194675 – Move bytecode cache-related filesystem code out of CodeCache

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tadeu Zagallo]

That code is only used for the bytecode-cache tests, so it should probably live in jsc.cpp rather than in the CodeCache.

[Tadeu Zagallo]

Created attachment 362064 [details]
Patch

[Saam Barati]

Comment on attachment 362064 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362064&action=review

This is a nice improvement, but I have one comment. Also you probably need to comment out more code on non-darwin platforms.

> Source/JavaScriptCore/runtime/CodeCache.cpp:206
> +    CachedBytecode cachedBytecode { WTFMove(result.first), result.second };
> +    key.source().provider().cacheBytecode(cachedBytecode);

Seems weird to have this function predicated on the cachePath above. Why not just make source provider tell you if it can cache or not?

[Saam Barati]

Comment on attachment 362064 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362064&action=review

> Source/JavaScriptCore/ChangeLog:8
> +        That code is only used for the bytecode-cache tests, so it should probably

no "probably"

> Source/JavaScriptCore/jsc.cpp:957
> +class CliSourceProvider : public StringSourceProvider {

I'd just call this "ShellSourceProvider" or "CommandLineSourceProvider"

[Tadeu Zagallo]

Created attachment 362108 [details]
Patch

[Tadeu Zagallo]

Comment on attachment 362064 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362064&action=review

>> Source/JavaScriptCore/runtime/CodeCache.cpp:206
>> +    key.source().provider().cacheBytecode(cachedBytecode);
> 
> Seems weird to have this function predicated on the cachePath above. Why not just make source provider tell you if it can cache or not?

Oops, this was an oversight. It shouldn't be darwin-specific either.

[Tadeu Zagallo]

Created attachment 362111 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 362111 [details]
Patch

Clearing flags on attachment: 362111

Committed r241612: <https://trac.webkit.org/changeset/241612>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48122052>

[Saam Barati]

Comment on attachment 362111 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362111&action=review

> Source/JavaScriptCore/runtime/CodeCache.cpp:199
>      std::pair<MallocPtr<uint8_t>, size_t> result = encodeCodeBlock(vm, key, codeBlock);

This patch made it so that we're now eagerly encoding CodeBlocks even if we can't cache it. And we're doing it as we remove entries from the cache. That's not the behavior we want

[WebKit Commit Bot]

Re-opened since this is blocked by bug 194762

[Tadeu Zagallo]

(In reply to Saam Barati from comment #10)
> Comment on attachment 362111 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362111&action=review
> 
> > Source/JavaScriptCore/runtime/CodeCache.cpp:199
> >      std::pair<MallocPtr<uint8_t>, size_t> result = encodeCodeBlock(vm, key, codeBlock);
> 
> This patch made it so that we're now eagerly encoding CodeBlocks even if we
> can't cache it. And we're doing it as we remove entries from the cache.
> That's not the behavior we want

Oops, that's not good... I'll upload a correct patch soon.

[Tadeu Zagallo]

Created attachment 362271 [details]
Patch

[Tadeu Zagallo]

I attached the new patch, which now has SourceProvider::useBytecodeCache to avoid the wasteful computations. I also merged the `flock` flags into `open` and added the `makeScopeExit` as Saam suggested in https://bugs.webkit.org/show_bug.cgi?id=194768.

[Tadeu Zagallo]

Created attachment 362281 [details]
Patch

Rebase

[Saam Barati]

Comment on attachment 362281 [details]
Patch

r=me. I think you need to also return true for useBytecodeCache in JSScriptSourceProvider

[Tadeu Zagallo]

(In reply to Saam Barati from comment #16)
> Comment on attachment 362281 [details]
> Patch
> 
> r=me. I think you need to also return true for useBytecodeCache in
> JSScriptSourceProvider

Good point. I'll update it. Thanks!

[Tadeu Zagallo]

Created attachment 362322 [details]
Patch

[Tadeu Zagallo]

(In reply to Tadeu Zagallo from comment #17)
> (In reply to Saam Barati from comment #16)
> > Comment on attachment 362281 [details]
> > Patch
> > 
> > r=me. I think you need to also return true for useBytecodeCache in
> > JSScriptSourceProvider
> 
> Good point. I'll update it. Thanks!

When I went to update JSScriptSourceProvider, I realized that we would still unnecessarily encode all the CodeBlocks for it, since it'd return `true` from `useBytecodeCache` in order to be able to provide a cache, but it never writes through the `cacheBytecode` callback.

I changed `cacheBytecode` to take a function that generates the cached bytecode, so I put it up for review again just in case.

[Saam Barati]

Comment on attachment 362322 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362322&action=review

r=me

> Source/JavaScriptCore/ChangeLog:9
> +        That code is only used for the bytecode-cache tests, so it should live in
> +        jsc.cpp rather than in the CodeCache.

You should make a couple comments about how this changes the interface for caching.

> Source/JavaScriptCore/parser/SourceProvider.h:120
> +        virtual void cacheBytecode(const BytecodeCacheGenerator&) const { }

I agree this is nicer than the canCache and solves the issue you found.

> Source/JavaScriptCore/runtime/CodeCache.cpp:202
> +    key.source().provider().cacheBytecode([&]() {

style nit: no need for "()"

[Tadeu Zagallo]

Created attachment 362374 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 362374 [details]
Patch for landing

Clearing flags on attachment: 362374

Committed r241758: <https://trac.webkit.org/changeset/241758>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.