Bug 192782 - [JSC] Cache bytecode to disk
Summary: [JSC] Cache bytecode to disk
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on: 193640
Blocks:
  Show dependency treegraph
 
Reported: 2018-12-17 15:48 PST by Tadeu Zagallo
Modified: 2019-01-22 10:51 PST (History)
13 users (show)

See Also:


Attachments
WIP (105.18 KB, patch)
2018-12-17 16:01 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (114.12 KB, patch)
2018-12-27 06:50 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (114.06 KB, patch)
2019-01-03 13:40 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (114.12 KB, patch)
2019-01-04 13:50 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (121.10 KB, patch)
2019-01-07 10:16 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (128.94 KB, patch)
2019-01-08 08:30 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (130.00 KB, patch)
2019-01-09 00:02 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (125.12 KB, patch)
2019-01-16 15:01 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (134.36 KB, patch)
2019-01-18 08:07 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (134.28 KB, patch)
2019-01-18 13:29 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (134.29 KB, patch)
2019-01-21 06:27 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (134.29 KB, patch)
2019-01-21 10:20 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch for landing (133.42 KB, patch)
2019-01-22 09:30 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>