That code is only used for the bytecode-cache tests, so it should probably live in jsc.cpp rather than in the CodeCache.
Created attachment 362064 [details] Patch
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?
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"
Created attachment 362108 [details] Patch
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.
Created attachment 362111 [details] Patch
Comment on attachment 362111 [details] Patch Clearing flags on attachment: 362111 Committed r241612: <https://trac.webkit.org/changeset/241612>
All reviewed patches have been landed. Closing bug.
<rdar://problem/48122052>
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
Re-opened since this is blocked by bug 194762
(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.
Created attachment 362271 [details] Patch
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.
Created attachment 362281 [details] Patch Rebase
Comment on attachment 362281 [details] Patch r=me. I think you need to also return true for useBytecodeCache in JSScriptSourceProvider
(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!
Created attachment 362322 [details] Patch
(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.
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 "()"
Created attachment 362374 [details] Patch for landing
Comment on attachment 362374 [details] Patch for landing Clearing flags on attachment: 362374 Committed r241758: <https://trac.webkit.org/changeset/241758>