Bug 70468 – DFG call optimization handling will fail if the call had been unlinked due to the callee being optimized

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Since the optimization trigger is likely to fire at roughly the same time for multiple code blocks, this leads to an unfortunate outcome where the optimization of one code block leads the others to forget about its existence.

[Filip Pizlo]

Created attachment 111720 [details]
the patch

[Oliver Hunt]

Comment on attachment 111720 [details]
the patch

lastSeenCallee isn't marked so this isn't safe, but also this may result in dead functions being kept alive longer than is strictly necessary.  I would almost consider going with a Weak<> ref rather than WriteBarrier<JSFunction> if we can get do it without a perf impact

[Filip Pizlo]

(In reply to comment #2)
> (From update of attachment 111720 [details])
> lastSeenCallee isn't marked so this isn't safe, but also this may result in dead functions being kept alive longer than is strictly necessary.  I would almost consider going with a Weak<> ref rather than WriteBarrier<JSFunction> if we can get do it without a perf impact

I'll change it to use the WeakReferenceHarvester approach.

[Oliver Hunt]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 111720 [details] [details])
> > lastSeenCallee isn't marked so this isn't safe, but also this may result in dead functions being kept alive longer than is strictly necessary.  I would almost consider going with a Weak<> ref rather than WriteBarrier<JSFunction> if we can get do it without a perf impact
> 
> I'll change it to use the WeakReferenceHarvester approach.

Righto

[Filip Pizlo]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 111720 [details] [details])
> > lastSeenCallee isn't marked so this isn't safe, but also this may result in dead functions being kept alive longer than is strictly necessary.  I would almost consider going with a Weak<> ref rather than WriteBarrier<JSFunction> if we can get do it without a perf impact
> 
> I'll change it to use the WeakReferenceHarvester approach.

Changed my mind.  As per earlier discussions, using a strong reference would not be a regression.  We currently unlink calls when we blow away all code.  If we blow away all code, then this reference (lastSeenCallee) would disappear from the GC trace since its owned by CodeBlock.

It's definitely worthwhile to make all stubs into weak references, and when they go stale, blow away the stubs.  Or in the case of DFG optimized code, trigger recompilation.

[Filip Pizlo]

Created attachment 111899 [details]
the patch

[Geoffrey Garen]

Comment on attachment 111899 [details]
the patch

r=me

Can we remove the callee field entirely now? It seems like it's a strict subset of lastSeenCallee.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/98065

[Filip Pizlo]

(In reply to comment #7)
> (From update of attachment 111899 [details])
> r=me
> 
> Can we remove the callee field entirely now? It seems like it's a strict subset of lastSeenCallee.

callee is a magic field - it's really just a pointer to the machine code containing the callee immediate.  We don't want to remove that since then we wouldn't be able to do linking/unlinking.