Summary: | Lazily decode cached bytecode | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tadeu Zagallo <tzagallo> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Tadeu Zagallo <tzagallo> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Tadeu Zagallo
2019-02-19 00:48:31 PST
Created attachment 362383 [details]
Patch
Created attachment 362495 [details]
Patch
Comment on attachment 362495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362495&action=review The main implementation idea seems good to me, but I think I've found some concurrency issues we should resolve. > Source/JavaScriptCore/ChangeLog:9 > + of decoding UnlinkedFunctionExecutable's CodeBlocks, we store their Instead of decoding => Instead of always eagerly decoding CodeBlocks => UnlinkedCodeBlocks > Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:281 > + > + m_isCached = false; To my first estimation, this is slightly wrong given concurrency issues in visitChildren. I think this should be: WTF::storeStoreFence(); m_isCached = false; writeBarrier(this); > Source/JavaScriptCore/runtime/CachedTypes.cpp:1821 > executable->finishCreation(decoder.vm()); > > - m_unlinkedCodeBlockForCall.decode(decoder, executable->m_unlinkedCodeBlockForCall, executable); > - m_unlinkedCodeBlockForConstruct.decode(decoder, executable->m_unlinkedCodeBlockForConstruct, executable); > + if (!m_unlinkedCodeBlockForCall.isEmpty() || !m_unlinkedCodeBlockForConstruct.isEmpty()) { > + executable->m_isCached = true; > + executable->m_decoder = &decoder; > + if (!m_unlinkedCodeBlockForCall.isEmpty()) > + executable->m_cachedCodeBlockForCallOffset = decoder.offsetOf(&m_unlinkedCodeBlockForCall); > + if (!m_unlinkedCodeBlockForConstruct.isEmpty()) > + executable->m_cachedCodeBlockForConstructOffset = decoder.offsetOf(&m_unlinkedCodeBlockForConstruct); > + } This also seems slightly wrong w.r.t concurrency issues. We usually treat finishCreation as a place that could allocate and invoke GC. Let's say that is true in this case (though it doesn't appear to be), this code is wrong since visitChildren may see m_isCached=false, but then this code may overwrite things that it's visiting. I think we can make this more robust by computing what we need and passing this information in inside the constructor above. > Source/JavaScriptCore/runtime/CachedTypes.cpp:2040 > + RefPtr<Decoder> decoder = adoptRef(new Decoder(vm, buffer, size)); Should be Ref. Also, WebKit style is to make this a create(...) function on Decoder itself. > Source/JavaScriptCore/runtime/CachedTypes.h:37 > +class Decoder : public RefCounted<Decoder> { Can we move various implementation functions in this class into the cpp file since they're mostly (all?) used from there anyways? Comment on attachment 362495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362495&action=review >> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:281 >> + m_isCached = false; > > To my first estimation, this is slightly wrong given concurrency issues in visitChildren. I think this should be: > > > WTF::storeStoreFence(); > m_isCached = false; > writeBarrier(this); Since there's a defer GC above, I was counting that visitChildren would not be called here. Is that a wrong assumption? >> Source/JavaScriptCore/runtime/CachedTypes.cpp:1821 >> + } > > This also seems slightly wrong w.r.t concurrency issues. We usually treat finishCreation as a place that could allocate and invoke GC. Let's say that is true in this case (though it doesn't appear to be), this code is wrong since visitChildren may see m_isCached=false, but then this code may overwrite things that it's visiting. I think we can make this more robust by computing what we need and passing this information in inside the constructor above. We defer GC during all of decoding, so I think this should be fine? Created attachment 363525 [details]
Patch
Comment on attachment 362495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362495&action=review >>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:281 >>> + m_isCached = false; >> >> To my first estimation, this is slightly wrong given concurrency issues in visitChildren. I think this should be: >> >> >> WTF::storeStoreFence(); >> m_isCached = false; >> writeBarrier(this); > > Since there's a defer GC above, I was counting that visitChildren would not be called here. Is that a wrong assumption? Yes. DeferGC ensures you won't safe point into the GC. It doesn't mean we're not already concurrently marking. Comment on attachment 363525 [details]
Patch
r- for the same reason as before.
Created attachment 363558 [details]
Patch
Comment on attachment 363558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363558&action=review > Source/JavaScriptCore/runtime/CachedTypes.cpp:2016 > + if (!cachedExecutable.unlinkedCodeBlockForCall().isEmpty()) Don’t you want to zero out these offsets if this check is falsey? Comment on attachment 363558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363558&action=review >> Source/JavaScriptCore/runtime/CachedTypes.cpp:2016 >> + if (!cachedExecutable.unlinkedCodeBlockForCall().isEmpty()) > > Don’t you want to zero out these offsets if this check is falsey? I already initialize m_unlinkedCodeBlockForCall and m_unlinkedCodeBlockForConstruct above, so they should already be zeroed, but I guess I should not rely on that since it's setting a different member of the union, which I guess is UB. Created attachment 363901 [details]
Patch for landing
Comment on attachment 363901 [details] Patch for landing Clearing flags on attachment: 363901 Committed r242605: <https://trac.webkit.org/changeset/242605> All reviewed patches have been landed. Closing bug. |