Serialize and cache the new bytecode to disk.
<rdar://problem/46084932>
Created attachment 357490 [details] WIP Serialization & Deserialization + dummy logic to write to /tmp for testing purposes.
Created attachment 358112 [details] Patch
Created attachment 358275 [details] Patch
Created attachment 358362 [details] Patch
Created attachment 358507 [details] Patch
Created attachment 358594 [details] Patch
Created attachment 358679 [details] Patch
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 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...
Created attachment 359316 [details] Patch
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 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`.
Created attachment 359489 [details] Patch
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
Created attachment 359530 [details] Patch
Comment on attachment 359530 [details] Patch r=me.
Comment on attachment 359530 [details] Patch Clearing flags on attachment: 359530 Committed r240210: <https://trac.webkit.org/changeset/240210>
All reviewed patches have been landed. Closing bug.
I actually think you'll need to have a shell script version of your runner since iOS can't run ruby.
broke iOS tests.
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] } ^
Created attachment 359688 [details] Patch
I converted the `bytecode-cache-test-helper` to bash, just double checking if everything works now.
Created attachment 359697 [details] Patch
Is this ready to be relanded?
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
Created attachment 359741 [details] Patch for landing
Comment on attachment 359741 [details] Patch for landing Clearing flags on attachment: 359741 Committed r240255: <https://trac.webkit.org/changeset/240255>
Committed r240257: <https://trac.webkit.org/changeset/240257>
<rdar://problem/47451479>