WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
192782
[JSC] Cache bytecode to disk
https://bugs.webkit.org/show_bug.cgi?id=192782
Summary
[JSC] Cache bytecode to disk
Tadeu Zagallo
Reported
2018-12-17 15:48:39 PST
Serialize and cache the new bytecode to disk.
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
Show Obsolete
(12)
View All
Add attachment
proposed patch, testcase, etc.
Tadeu Zagallo
Comment 1
2018-12-17 15:49:26 PST
<
rdar://problem/46084932
>
Tadeu Zagallo
Comment 2
2018-12-17 16:01:27 PST
Created
attachment 357490
[details]
WIP Serialization & Deserialization + dummy logic to write to /tmp for testing purposes.
Tadeu Zagallo
Comment 3
2018-12-27 06:50:17 PST
Created
attachment 358112
[details]
Patch
Tadeu Zagallo
Comment 4
2019-01-03 13:40:55 PST
Created
attachment 358275
[details]
Patch
Tadeu Zagallo
Comment 5
2019-01-04 13:50:58 PST
Created
attachment 358362
[details]
Patch
Tadeu Zagallo
Comment 6
2019-01-07 10:16:12 PST
Created
attachment 358507
[details]
Patch
Tadeu Zagallo
Comment 7
2019-01-08 08:30:43 PST
Created
attachment 358594
[details]
Patch
Tadeu Zagallo
Comment 8
2019-01-09 00:02:06 PST
Created
attachment 358679
[details]
Patch
Keith Miller
Comment 9
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?
Tadeu Zagallo
Comment 10
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...
Tadeu Zagallo
Comment 11
2019-01-16 15:01:49 PST
Created
attachment 359316
[details]
Patch
Keith Miller
Comment 12
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.
Tadeu Zagallo
Comment 13
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`.
Tadeu Zagallo
Comment 14
2019-01-18 08:07:46 PST
Created
attachment 359489
[details]
Patch
EWS Watchlist
Comment 15
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
Tadeu Zagallo
Comment 16
2019-01-18 13:29:35 PST
Created
attachment 359530
[details]
Patch
Keith Miller
Comment 17
2019-01-19 11:32:11 PST
Comment on
attachment 359530
[details]
Patch r=me.
WebKit Commit Bot
Comment 18
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
>
WebKit Commit Bot
Comment 19
2019-01-20 03:20:33 PST
All reviewed patches have been landed. Closing bug.
Keith Miller
Comment 20
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.
Saam Barati
Comment 21
2019-01-20 19:59:22 PST
broke iOS tests.
Michael Catanzaro
Comment 22
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] } ^
Tadeu Zagallo
Comment 23
2019-01-21 06:27:51 PST
Created
attachment 359688
[details]
Patch
Tadeu Zagallo
Comment 24
2019-01-21 06:43:13 PST
I converted the `bytecode-cache-test-helper` to bash, just double checking if everything works now.
Tadeu Zagallo
Comment 25
2019-01-21 10:20:17 PST
Created
attachment 359697
[details]
Patch
Keith Miller
Comment 26
2019-01-22 07:12:55 PST
Is this ready to be relanded?
WebKit Commit Bot
Comment 27
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
Tadeu Zagallo
Comment 28
2019-01-22 09:30:16 PST
Created
attachment 359741
[details]
Patch for landing
WebKit Commit Bot
Comment 29
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
>
WebKit Commit Bot
Comment 30
2019-01-22 10:00:24 PST
All reviewed patches have been landed. Closing bug.
Tadeu Zagallo
Comment 31
2019-01-22 10:33:27 PST
Committed
r240257
: <
https://trac.webkit.org/changeset/240257
>
Tadeu Zagallo
Comment 32
2019-01-22 10:51:11 PST
<
rdar://problem/47451479
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug