Bug 192782

Summary: [JSC] Cache bytecode to disk
Product: WebKit Reporter: Tadeu Zagallo <tzagallo>
Component: JavaScriptCoreAssignee: Tadeu Zagallo <tzagallo>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, dbates, ews, keith_miller, mark.lam, mathias, mcatanzaro, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 193640    
Bug Blocks:    
Attachments:
Description Flags
WIP
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Tadeu Zagallo 2018-12-17 15:48:39 PST
Serialize and cache the new bytecode to disk.
Comment 1 Tadeu Zagallo 2018-12-17 15:49:26 PST
<rdar://problem/46084932>
Comment 2 Tadeu Zagallo 2018-12-17 16:01:27 PST
Created attachment 357490 [details]
WIP

Serialization & Deserialization + dummy logic to write to /tmp for testing purposes.
Comment 3 Tadeu Zagallo 2018-12-27 06:50:17 PST
Created attachment 358112 [details]
Patch
Comment 4 Tadeu Zagallo 2019-01-03 13:40:55 PST
Created attachment 358275 [details]
Patch
Comment 5 Tadeu Zagallo 2019-01-04 13:50:58 PST
Created attachment 358362 [details]
Patch
Comment 6 Tadeu Zagallo 2019-01-07 10:16:12 PST
Created attachment 358507 [details]
Patch
Comment 7 Tadeu Zagallo 2019-01-08 08:30:43 PST
Created attachment 358594 [details]
Patch
Comment 8 Tadeu Zagallo 2019-01-09 00:02:06 PST
Created attachment 358679 [details]
Patch
Comment 9 Keith Miller 2019-01-15 20:49:14 PST
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?
Comment 10 Tadeu Zagallo 2019-01-16 14:42:12 PST
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...
Comment 11 Tadeu Zagallo 2019-01-16 15:01:49 PST
Created attachment 359316 [details]
Patch
Comment 12 Keith Miller 2019-01-17 18:01:20 PST
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.
Comment 13 Tadeu Zagallo 2019-01-18 00:07:39 PST
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`.
Comment 14 Tadeu Zagallo 2019-01-18 08:07:46 PST
Created attachment 359489 [details]
Patch
Comment 15 Build Bot 2019-01-18 13:21:12 PST
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
Comment 16 Tadeu Zagallo 2019-01-18 13:29:35 PST
Created attachment 359530 [details]
Patch
Comment 17 Keith Miller 2019-01-19 11:32:11 PST
Comment on attachment 359530 [details]
Patch

r=me.
Comment 18 WebKit Commit Bot 2019-01-20 03:20:31 PST
Comment on attachment 359530 [details]
Patch

Clearing flags on attachment: 359530

Committed r240210: <https://trac.webkit.org/changeset/240210>
Comment 19 WebKit Commit Bot 2019-01-20 03:20:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Keith Miller 2019-01-20 19:43:00 PST
I actually think you'll need to have a shell script version of your runner since iOS can't run ruby.
Comment 21 Saam Barati 2019-01-20 19:59:22 PST
broke iOS tests.
Comment 22 Michael Catanzaro 2019-01-20 21:06:48 PST
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]
 }
 ^
Comment 23 Tadeu Zagallo 2019-01-21 06:27:51 PST
Created attachment 359688 [details]
Patch
Comment 24 Tadeu Zagallo 2019-01-21 06:43:13 PST
I converted the `bytecode-cache-test-helper` to bash, just double checking if everything works now.
Comment 25 Tadeu Zagallo 2019-01-21 10:20:17 PST
Created attachment 359697 [details]
Patch
Comment 26 Keith Miller 2019-01-22 07:12:55 PST
Is this ready to be relanded?
Comment 27 WebKit Commit Bot 2019-01-22 08:14:42 PST
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 commit-queue@webkit.org...
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
Comment 28 Tadeu Zagallo 2019-01-22 09:30:16 PST
Created attachment 359741 [details]
Patch for landing
Comment 29 WebKit Commit Bot 2019-01-22 10:00:21 PST
Comment on attachment 359741 [details]
Patch for landing

Clearing flags on attachment: 359741

Committed r240255: <https://trac.webkit.org/changeset/240255>
Comment 30 WebKit Commit Bot 2019-01-22 10:00:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 31 Tadeu Zagallo 2019-01-22 10:33:27 PST
Committed r240257: <https://trac.webkit.org/changeset/240257>
Comment 32 Tadeu Zagallo 2019-01-22 10:51:11 PST
<rdar://problem/47451479>