Bug 194810 – Lazily decode cached bytecode

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tadeu Zagallo]

Like lazy parsing, we should pause at code block boundaries.

[Tadeu Zagallo]

Created attachment 362383 [details]
Patch

[Tadeu Zagallo]

Created attachment 362495 [details]
Patch

[Saam Barati]

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?

[Tadeu Zagallo]

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?

[Tadeu Zagallo]

Created attachment 363525 [details]
Patch

[Saam Barati]

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.

[Saam Barati]

Comment on attachment 363525 [details]
Patch

r- for the same reason as before.

[Tadeu Zagallo]

Created attachment 363558 [details]
Patch

[Saam Barati]

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?

[Tadeu Zagallo]

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.

[Tadeu Zagallo]

Created attachment 363901 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 363901 [details]
Patch for landing

Clearing flags on attachment: 363901

Committed r242605: <https://trac.webkit.org/changeset/242605>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48684573>