Bug 194810

Summary: Lazily decode cached bytecode
Product: WebKit Reporter: Tadeu Zagallo <tzagallo>
Component: JavaScriptCoreAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Tadeu Zagallo 2019-02-19 00:48:31 PST
Like lazy parsing, we should pause at code block boundaries.
Comment 1 Tadeu Zagallo 2019-02-19 06:56:13 PST
Created attachment 362383 [details]
Patch
Comment 2 Tadeu Zagallo 2019-02-20 06:16:36 PST
Created attachment 362495 [details]
Patch
Comment 3 Saam Barati 2019-02-28 12:58:35 PST
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 4 Tadeu Zagallo 2019-02-28 23:26:49 PST
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?
Comment 5 Tadeu Zagallo 2019-03-04 10:30:10 PST
Created attachment 363525 [details]
Patch
Comment 6 Saam Barati 2019-03-04 11:47:05 PST
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 7 Saam Barati 2019-03-04 11:48:04 PST
Comment on attachment 363525 [details]
Patch

r- for the same reason as before.
Comment 8 Tadeu Zagallo 2019-03-04 15:38:52 PST
Created attachment 363558 [details]
Patch
Comment 9 Saam Barati 2019-03-07 10:05:55 PST
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 10 Tadeu Zagallo 2019-03-07 10:58:30 PST
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.
Comment 11 Tadeu Zagallo 2019-03-07 11:05:16 PST
Created attachment 363901 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2019-03-07 11:43:49 PST
Comment on attachment 363901 [details]
Patch for landing

Clearing flags on attachment: 363901

Committed r242605: <https://trac.webkit.org/changeset/242605>
Comment 13 WebKit Commit Bot 2019-03-07 11:43:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-03-07 12:00:59 PST
<rdar://problem/48684573>