[webkit-reviews] review denied: [Bug 204919] The compiler thread should not adjust Identifier refCounts. : [Attachment 384960] proposed patch.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 204919: The compiler thread should not adjust Identifier refCounts.
https://bugs.webkit.org/show_bug.cgi?id=204919

Attachment 384960: proposed patch.

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




--- Comment #2 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 384960
  --> https://bugs.webkit.org/attachment.cgi?id=384960
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=384960&action=review

r- because of bug I noticed

> Source/JavaScriptCore/bytecode/GetByStatus.h:98
> +    using IdentifierKeepAlive = std::function<void(Box<Identifier>)>;

let's use WTF::SharedTask so that we don't repeatedly copy around the
std::function struct. Instead, we just ref count it.

> Source/JavaScriptCore/dfg/DFGPlan.h:121
> +	   m_identifiersKeptAliveForCleanUp.append(WTFMove(identifier));

let's add a branch here:
if (identifier)
    m_identifiersKeptAliveForCleanUp.append(WTFMove(identifier));

So we don't waste memory in the vector for null entries

> Source/JavaScriptCore/dfg/DFGWorklist.cpp:418
> +	   m_cancelledPlans.clear();

I think this is wrong, unfortunately. Imagine we're in an API scenario (or web
worker scenario). We have multiple VMs. We're now deleting Plans for other VMs.
This will lead to the race happening still.

I suggest writing a function like:
deleteCancelledPlansForVM(VM&)

And I'll suggest again what I suggested in person, which is to assert that
we're holding the current VM lock.