Bug 86306 – DFG should be able to optimize foo.apply(bar, arguments)

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 141589 [details]
work in progress

Still testing this monstrosity.

[Filip Pizlo]

Created attachment 141597 [details]
the patch

[Filip Pizlo]

Created attachment 141610 [details]
the patch

Made some refinements to the way that CFG simplification treats captured variables.  Previously I thought that it would be fine for CFG simplification to not do any fix-up to captured vars after merging basic blocks.  But now I think that it's cleaner if we preserve the invariant that even for captured vars, a GetLocal refers to the first prior access to the same variable, thereby ensuring that (1) we have a clean way of seeing the liveness of variables and (2) the unification-based variable splitting "just works".  Of course, the latter is only needed prior to CFG simplification, but I like the idea of CFG simplification producing a graph that is strictly more tidy than the one it received.

[Oliver Hunt]

Comment on attachment 141610 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:556
> +                if (false && node.variableAccessData()->isCaptured())
> +                    break;
> +                

false? this seems bogus :D

> Source/JavaScriptCore/dfg/DFGCapabilities.h:-198
> -        return false;

What's this change?

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 141610 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141610&action=review
> 
> > Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:556
> > +                if (false && node.variableAccessData()->isCaptured())
> > +                    break;
> > +                
> 
> false? this seems bogus :D
> 

Ooops!  I meant to kill that entire if statement rather than just turn it into a no-op.

> > Source/JavaScriptCore/dfg/DFGCapabilities.h:-198
> > -        return false;
> 
> What's this change?

That return was redundant.  If you look at the surrounding code, there's a return false below that this would fall-through to anyway.

[Filip Pizlo]

Created attachment 141611 [details]
the patch

Removed the "if (false && ..." thingy that Oliver pointed out.

[Darin Adler]

Comment on attachment 141611 [details]
the patch

For some reason this patch doesn’t apply so the EWS is not working.

[Filip Pizlo]

(In reply to comment #7)
> (From update of attachment 141611 [details])
> For some reason this patch doesn’t apply so the EWS is not working.

All is well - this patch is for the dfgopt branch.  So EWS won't be able to do anything about it, since the branch has diverged just enough that new DFG patches are unlikely to apply on trunk.

[Filip Pizlo]

Created attachment 141619 [details]
the patch

I forgot that optimizing op_call_varargs to use arguments directly implies that we need to first check that the arguments object has not been created.  This revised version of the patch does this, though imperfectly; the only hole (use of static speculation) will be fixed in https://bugs.webkit.org/show_bug.cgi?id=86327.

[Gavin Barraclough]

Comment on attachment 141619 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:72
> +                        

You probably didn't mean to do this.

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:45
> +    UNUSED_PARAM(result);

This is redundant (*shrug* :-) ).

> Source/JavaScriptCore/dfg/DFGDriver.cpp:53
> +    

Probably want to revert this file.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:948
> +    

Again probably want to revert.

[Filip Pizlo]

(In reply to comment #10)
> (From update of attachment 141619 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141619&action=review
> 
> > Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:72
> > +                        
> 
> You probably didn't mean to do this.
> 
> > Source/JavaScriptCore/dfg/DFGCapabilities.cpp:45
> > +    UNUSED_PARAM(result);
> 
> This is redundant (*shrug* :-) ).
> 
> > Source/JavaScriptCore/dfg/DFGDriver.cpp:53
> > +    
> 
> Probably want to revert this file.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:948
> > +    
> 
> Again probably want to revert.

Thanks for catching the redundancies!  I've removed them.

[Filip Pizlo]

(In reply to comment #10)
> (From update of attachment 141619 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141619&action=review
> 
> > Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:72
> > +                        
> 
> You probably didn't mean to do this.
> 
> > Source/JavaScriptCore/dfg/DFGCapabilities.cpp:45
> > +    UNUSED_PARAM(result);
> 
> This is redundant (*shrug* :-) ).

Ooops!  Nope, it isn't.  The issue is that the only other use of it is the ASSERT.  So I have a choice between switching the ASSERT to ASSERT_UNUSED or keeping the UNUSED_PARAM.  I like keeping the UNUSED_PARAM better because the whole point of passing the result into this method is so that the verbose mode can use it to print things.  Hence, the UNUSED_PARAM on the #else case of the DEBUG_VERBOSE check, as opposed to an ASSERT_UNUSED that is unguarded by any other checks, is serving a useful purpose: if when building with DEBUG_VERBOSE mode the compiler finds that result is unused, then it should flag an error, which will hopefully serve as a signal to the hacker that he should remove the result argument altogether.

[Filip Pizlo]

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

[Filip Pizlo]

Merged in http://trac.webkit.org/changeset/118270