[webkit-reviews] review denied: [Bug 194810] Lazily decode cached bytecode : [Attachment 362495] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Tadeu Zagallo <tzagallo at apple.com>'s
request for review:
Bug 194810: Lazily decode cached bytecode
https://bugs.webkit.org/show_bug.cgi?id=194810

Attachment 362495: Patch

https://bugs.webkit.org/attachment.cgi?id=362495&action=review




--- Comment #3 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 362495
  --> https://bugs.webkit.org/attachment.cgi?id=362495
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?