Bug 192782 – [JSC] Cache bytecode to disk

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

Serialize and cache the new bytecode to disk.

[Tadeu Zagallo]

<rdar://problem/46084932>

[Tadeu Zagallo]

Created attachment 357490 [details]
WIP

Serialization & Deserialization + dummy logic to write to /tmp for testing purposes.

[Tadeu Zagallo]

Created attachment 358112 [details]
Patch

[Tadeu Zagallo]

Created attachment 358275 [details]
Patch

[Tadeu Zagallo]

Created attachment 358362 [details]
Patch

[Tadeu Zagallo]

Created attachment 358507 [details]
Patch

[Tadeu Zagallo]

Created attachment 358594 [details]
Patch

[Tadeu Zagallo]

Created attachment 358679 [details]
Patch

[Keith Miller]

Comment on attachment 358679 [details]
Patch

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

I think you need to name things more in line with WebKit style. Overall it looks pretty good, I'll take a closer look after the style update.

> Source/JavaScriptCore/ChangeLog:14
> +

Can you put a description of how you designed the hierarchy of CachedTypes? That will probably help people digest this in the future.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:53
> +struct SourceTypeImpl<T, std::enable_if_t<!std::is_fundamental<T>::value && !std::is_same<typename T::SourceType_, void>::value>> {

why do you need the std::is_fundamental<T>::value? Wouldn't typename T::SourceType_ fail for any fundamental type?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:63
> +

You might also want WTF_FORBID_HEAP_ALLOCATION.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:139
> +

Ditto.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:392
> +template<typename T, typename ST = SourceType<T>>

We should use full names for these per WebKit style specifically for things like Source. The T is probably fine since it's pretty obvious.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:409
> +template<typename T, size_t IC = 0, typename OH = CrashOnOverflow>

Ditto for IC and OH. They should be something like InlineCapacity and OverflowHandler.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:431
> +template<typename Fst, typename Snd>

Ditto here and elsewhere.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:983
> +        if (classInfo == SymbolTable::info()) {

Nit: these could all be:

if (auto* symbolTable = jsDynamicCast<SymbolTable*>(cell))
...

> Source/JavaScriptCore/runtime/CachedTypes.cpp:1068
> +        Vector<uint8_t, 0, UnsafeVectorOverflow> instructionsVector;

Why does this need do be unsafe?

[Tadeu Zagallo]

Comment on attachment 358679 [details]
Patch

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

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:53
>> +struct SourceTypeImpl<T, std::enable_if_t<!std::is_fundamental<T>::value && !std::is_same<typename T::SourceType_, void>::value>> {
> 
> why do you need the std::is_fundamental<T>::value? Wouldn't typename T::SourceType_ fail for any fundamental type?

The compiler doesn't like e.g. `int::SourceType_`

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:392
>> +template<typename T, typename ST = SourceType<T>>
> 
> We should use full names for these per WebKit style specifically for things like Source. The T is probably fine since it's pretty obvious.

Yeah, I started with SourceType, but it conflicted with the top-level type. I'll change it to Source.

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:409
>> +template<typename T, size_t IC = 0, typename OH = CrashOnOverflow>
> 
> Ditto for IC and OH. They should be something like InlineCapacity and OverflowHandler.

Oops, this one I just copied from WTF/Vector and didn't even think about it. I'll fix it.

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:1068
>> +        Vector<uint8_t, 0, UnsafeVectorOverflow> instructionsVector;
> 
> Why does this need do be unsafe?

It has to match the OverflowHandler in the InstructionStream. I'm not sure why that one is unsafe though...

[Tadeu Zagallo]

Created attachment 359316 [details]
Patch

[Keith Miller]

Comment on attachment 359316 [details]
Patch

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

r- on some more changes. I should have looked closer last time. I'll take another look once you've addressed my comments.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:82
> +        m_offset = WTF::roundUpToMultipleOf(sizeof(void*), m_offset);

does this work on 32-bit? isn't alignment wrong for things that need to be 8-byte aligned? It seems like this should take an alignment.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:132
> +    uint8_t* m_buffer;

This should be MallocPtr<uint8_t>.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:231
> +template<typename Source, typename Dst>

Dst seems unused?

> Source/JavaScriptCore/runtime/CachedTypes.cpp:267
> +template<typename Source, typename Dst>

Ditto.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:272
> +    ptrdiff_t offset { s_invalidOffset };

This should be at the bottom of the class per WebKit style.

Also, why is it public? it should probably just have a getter and be prefixed with m_

> Source/JavaScriptCore/runtime/CachedTypes.cpp:316
> +    bool isEmpty;

Ditto on location/naming/visibility of member.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:329
> +        T* cachedObject = this->template allocate<T>(encoder);

I think this can just be "allocate<T>(encoder);"

> Source/JavaScriptCore/runtime/CachedTypes.cpp:344
> +        Source* ptr = operator->()->decode(decoder, args...);

Let's not invoke the operator and make another method get() or something that the operator-> also calls.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:353
> +        return this->template buffer<T>();

I think this can just be "return buffer<T>();"

> Source/JavaScriptCore/runtime/CachedTypes.cpp:360
> +        return this->template buffer<T>();

Ditto.

> Source/JavaScriptCore/runtime/CachedTypes.cpp:366
> +    CachedPtr<T, Source> ptr;

Ditto on naming/visibility/location

> Source/JavaScriptCore/runtime/CachedTypes.cpp:667
> +struct CachedRareData : public CachedObject<UnlinkedCodeBlock::RareData, CachedRareData> {

Can we name this CachedCodeBlockRaredata? It's not clear which objects rare data it is right now.

[Tadeu Zagallo]

Comment on attachment 359316 [details]
Patch

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

I'll give it another full pass to adjust it to WebKit style.

>> Source/JavaScriptCore/runtime/CachedTypes.cpp:329
>> +        T* cachedObject = this->template allocate<T>(encoder);
> 
> I think this can just be "allocate<T>(encoder);"

Apparently this doesn't work when the base class is a template, it has to either be `this->allocate` or `Base<...>::allocate`.

[Tadeu Zagallo]

Created attachment 359489 [details]
Patch

[EWS Watchlist]

Comment on attachment 359489 [details]
Patch

Attachment 359489 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10799692

New failing tests:
microbenchmarks/deltablue-varargs.js.bytecode-cache
apiTests

[Tadeu Zagallo]

Created attachment 359530 [details]
Patch

[Keith Miller]

Comment on attachment 359530 [details]
Patch

r=me.

[WebKit Commit Bot]

Comment on attachment 359530 [details]
Patch

Clearing flags on attachment: 359530

Committed r240210: <https://trac.webkit.org/changeset/240210>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Keith Miller]

I actually think you'll need to have a shell script version of your runner since iOS can't run ruby.

[Saam Barati]

broke iOS tests.

[Michael Catanzaro]

Also, be sure to RELEASE_ASSERT_NOT_REACHED() if the end of a function is unreachable:

[2184/4122] Building CXX object Source/JavaScriptCo...Core/unified-sources/UnifiedSource-f2e18ffc-4.cpp.o
In file included from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f2e18ffc-4.cpp:1:
../../Source/JavaScriptCore/runtime/CachedTypes.cpp: In member function ‘std::pair<JSC::SourceCodeKey, JSC::UnlinkedCodeBlock*> JSC::GenericCacheEntry::decode(JSC::Decoder&) const’:
../../Source/JavaScriptCore/runtime/CachedTypes.cpp:1985:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^

[Tadeu Zagallo]

Created attachment 359688 [details]
Patch

[Tadeu Zagallo]

I converted the `bytecode-cache-test-helper` to bash, just double checking if everything works now.

[Tadeu Zagallo]

Created attachment 359697 [details]
Patch

[Keith Miller]

Is this ready to be relanded?

[WebKit Commit Bot]

Comment on attachment 359697 [details]
Patch

Rejecting attachment 359697 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 359697, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=359697&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=192782&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 359697 from bug 192782.
Fetching: https://bugs.webkit.org/attachment.cgi?id=359697
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 42 diffs from patch file(s).
patching file Source/JavaScriptCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WTF/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/JavaScriptCore/CMakeLists.txt
patching file Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
patching file Source/JavaScriptCore/Sources.txt
patching file Source/JavaScriptCore/builtins/BuiltinNames.cpp
patching file Source/JavaScriptCore/builtins/BuiltinNames.h
patching file Source/JavaScriptCore/bytecode/CodeBlock.cpp
patching file Source/JavaScriptCore/bytecode/CodeBlock.h
patching file Source/JavaScriptCore/bytecode/HandlerInfo.h
patching file Source/JavaScriptCore/bytecode/InstructionStream.h
patching file Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h
patching file Source/JavaScriptCore/bytecode/UnlinkedEvalCodeBlock.h
patching file Source/JavaScriptCore/bytecode/UnlinkedFunctionCodeBlock.h
patching file Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h
patching file Source/JavaScriptCore/bytecode/UnlinkedGlobalCodeBlock.h
patching file Source/JavaScriptCore/bytecode/UnlinkedMetadataTable.h
patching file Source/JavaScriptCore/bytecode/UnlinkedModuleProgramCodeBlock.h
patching file Source/JavaScriptCore/bytecode/UnlinkedProgramCodeBlock.h
patching file Source/JavaScriptCore/interpreter/Interpreter.cpp
patching file Source/JavaScriptCore/jsc.cpp
patching file Source/JavaScriptCore/parser/SourceCode.h
patching file Source/JavaScriptCore/parser/SourceCodeKey.h
patching file Source/JavaScriptCore/parser/UnlinkedSourceCode.h
patching file Source/JavaScriptCore/parser/VariableEnvironment.h
patching file Source/JavaScriptCore/runtime/CachedTypes.cpp
patching file Source/JavaScriptCore/runtime/CachedTypes.h
patching file Source/JavaScriptCore/runtime/CodeCache.cpp
patching file Source/JavaScriptCore/runtime/CodeCache.h
patching file Source/JavaScriptCore/runtime/JSBigInt.cpp
patching file Source/JavaScriptCore/runtime/JSBigInt.h
patching file Source/JavaScriptCore/runtime/Options.cpp
Hunk #1 succeeded at 517 (offset -7 lines).
patching file Source/JavaScriptCore/runtime/Options.h
Hunk #1 FAILED at 509.
1 out of 1 hunk FAILED -- saving rejects to file Source/JavaScriptCore/runtime/Options.h.rej
patching file Source/JavaScriptCore/runtime/RegExp.h
patching file Source/JavaScriptCore/runtime/ScopedArgumentsTable.h
patching file Source/JavaScriptCore/runtime/StackFrame.h
patching file Source/JavaScriptCore/runtime/StructureInlines.h
patching file Source/JavaScriptCore/runtime/SymbolTable.h
patching file Source/WTF/wtf/BitVector.h
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/Scripts/jsc-stress-test-helpers/bytecode-cache-test-helper.sh
patching file Tools/Scripts/run-jsc-stress-tests

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/10839393

[Tadeu Zagallo]

Created attachment 359741 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 359741 [details]
Patch for landing

Clearing flags on attachment: 359741

Committed r240255: <https://trac.webkit.org/changeset/240255>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tadeu Zagallo]

Committed r240257: <https://trac.webkit.org/changeset/240257>

[Tadeu Zagallo]

<rdar://problem/47451479>