Bug 155391 – We should be able to eliminate cloned arguments objects that use the length property

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

We should be able to eliminate cloned arguments objects that use the length property

[Keith Miller]

Created attachment 273989 [details]
Patch

[WebKit Commit Bot]

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.

[Keith Miller]

Created attachment 273991 [details]
Benchmark results

!s appear to be noise on reruns.

[Geoffrey Garen]

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.

[Saam Barati]

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"

[Keith Miller]

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.

[Keith Miller]

Created attachment 274004 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 274004 [details]
Patch for landing

Clearing flags on attachment: 274004

Committed r198154: <http://trac.webkit.org/changeset/198154>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Looks like this was 2-5% improvement on Speedometer!