Bug 155391

Summary: We should be able to eliminate cloned arguments objects that use the length property
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, mark.lam, msaboff, rniwa, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Benchmark results
none
Patch for landing none

Description Keith Miller 2016-03-12 08:13:08 PST
We should be able to eliminate cloned arguments objects that use the length property
Comment 1 Keith Miller 2016-03-14 10:41:31 PDT
Created attachment 273989 [details]
Patch
Comment 2 WebKit Commit Bot 2016-03-14 10:44:04 PDT
Attachment 273989 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:122:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:129:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Keith Miller 2016-03-14 10:47:21 PDT
Created attachment 273991 [details]
Benchmark results

!s appear to be noise on reruns.
Comment 4 Geoffrey Garen 2016-03-14 11:19:00 PDT
Comment on attachment 273989 [details]
Patch

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

You should add a performance regression test, and also some adversarial tests that modify the structure of the arguments object in interesting ways and verify that doing so still gets the right behavior.

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:434
> +                    // Meh, this is kind of hackish - we use an Identity so that we can reuse the
> +                    // getArrayLength() helper.

I don't think you need this comment. Converting to identity is a pretty common idiom.

> Source/JavaScriptCore/runtime/ClonedArguments.cpp:44
>  ClonedArguments* ClonedArguments::createEmpty(

All of the create functions in this file should end by ASSERTing that their structure is equal to globalObject->clonedArgumentsStructure().

> Source/JavaScriptCore/runtime/ClonedArguments.cpp:140
> +    // FIXME: we should do a better job at choosing the indexing type for cloned arguments. We always choose contiguous
> +    // since it enables arguments elimination with the length property. https://bugs.webkit.org/show_bug.cgi?id=155396

This comment would be better if it just explained the rationale for the current way of doing things, rather than half-explaining and half-criticizing:

// We use contiguous storage because optimizations in the FTL assume that cloned arguments creation always produces the same initial structure.

It's not necessarily obvious whether this should change or not. We should reserve FIXME for cases when we know that something should change.
Comment 5 Saam Barati 2016-03-14 11:37:13 PDT
Comment on attachment 273989 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:126
> +            if (m_candidates.remove(edge.node()) && verbose)
> +                dataLog("eliminating candidate: ", edge.node(), " because it escapes from: ", source, "\n");

Style: I'm not a big fan of having a logging branch and a necessary operation in the same "if" statement.
I think it'd be better to assign the result to a local and branch on that and "verbose"
Comment 6 Keith Miller 2016-03-14 12:52:03 PDT
Comment on attachment 273989 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:126
>> +                dataLog("eliminating candidate: ", edge.node(), " because it escapes from: ", source, "\n");
> 
> Style: I'm not a big fan of having a logging branch and a necessary operation in the same "if" statement.
> I think it'd be better to assign the result to a local and branch on that and "verbose"

Fair enough, fixed.

>> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:434
>> +                    // getArrayLength() helper.
> 
> I don't think you need this comment. Converting to identity is a pretty common idiom.

Removed.

>> Source/JavaScriptCore/runtime/ClonedArguments.cpp:44
>>  ClonedArguments* ClonedArguments::createEmpty(
> 
> All of the create functions in this file should end by ASSERTing that their structure is equal to globalObject->clonedArgumentsStructure().

Added.

>> Source/JavaScriptCore/runtime/ClonedArguments.cpp:140
>> +    // since it enables arguments elimination with the length property. https://bugs.webkit.org/show_bug.cgi?id=155396
> 
> This comment would be better if it just explained the rationale for the current way of doing things, rather than half-explaining and half-criticizing:
> 
> // We use contiguous storage because optimizations in the FTL assume that cloned arguments creation always produces the same initial structure.
> 
> It's not necessarily obvious whether this should change or not. We should reserve FIXME for cases when we know that something should change.

Ok, I changed it to your comment.
Comment 7 Keith Miller 2016-03-14 12:54:57 PDT
Created attachment 274004 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2016-03-14 13:55:02 PDT
Comment on attachment 274004 [details]
Patch for landing

Clearing flags on attachment: 274004

Committed r198154: <http://trac.webkit.org/changeset/198154>
Comment 9 WebKit Commit Bot 2016-03-14 13:55:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Ryosuke Niwa 2016-03-14 22:55:46 PDT
Looks like this was 2-5% improvement on Speedometer!