Bug 164502 – [JSC] Avoid cloned arguments allocation in ArrayPrototype methods

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

[Yusuke Suzuki]

...

[Yusuke Suzuki]

While FTL can drop this, it is good if we can avoid this thing in LLInt, Baseline and DFG.

[Yusuke Suzuki]

It seems that this will give us 7.4% improvement in ES6SampleBench/Basic! 164.36ms vs 152.98ms

[Yusuke Suzuki]

Created attachment 294187 [details]
Patch

WIP

[Build Bot]

Comment on attachment 294187 [details]
Patch

Attachment 294187 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2479692

New failing tests:
js/array-fill.html

[Build Bot]

Created attachment 294195 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 294187 [details]
Patch

Attachment 294187 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2479697

New failing tests:
js/array-fill.html

[Build Bot]

Created attachment 294196 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Yusuke Suzuki]

Created attachment 294197 [details]
Patch

WIP

[Yusuke Suzuki]

Related https://bugs.webkit.org/show_bug.cgi?id=143077

[Yusuke Suzuki]

Created attachment 294210 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 294211 [details]
Patch

WIP

[Build Bot]

Comment on attachment 294211 [details]
Patch

Attachment 294211 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2480705

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 294213 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Yusuke Suzuki]

Created attachment 294214 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 294215 [details]
Patch

[Yusuke Suzuki]

The patch is ready.

[Saam Barati]

Comment on attachment 294215 [details]
Patch

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

r=me
However, I'd like to see some tests for varargs inlined callees.

> Source/JavaScriptCore/ChangeLog:60
> +        All summary.
> +            Baseline:
> +                summary:            164.34 ms +- 5.01 ms
> +            Patched:
> +                summary:            157.26 ms +- 5.96 ms

Nice!

> Source/JavaScriptCore/builtins/ArrayPrototype.js:314
> +    var relativeStart = @toInteger(@argument(1));

toInteger of undefined is zero?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6784
> +    JSValueRegs resultRegs = result.regs();
> +    m_jit.load32(CCallHelpers::payloadFor(CallFrameSlot::argumentCount), argumentCountGPR);

This doesn't seem correct if we've been inlined. Can you add tests?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6785
> +    auto argumentOutOfBounds = m_jit.branch32(CCallHelpers::LessThanOrEqual, argumentCountGPR, CCallHelpers::Imm32(node->argumentIndex()));

Nit: This could be TrustedImm32 since it comes from our builtins.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5168
> +        LValue argumentCount = m_out.load32(payloadFor(CallFrameSlot::argumentCount));

Ditto, this does't look correct for inlining (I guess varargs inlinled callee is when this could come up because of your bytecode parser rules)

> Source/JavaScriptCore/jit/JITOpcodes.cpp:511
> +    Jump argumentOutOfBounds = branch32(LessThanOrEqual, regT0, Imm32(index));

Ditto nit: Could be TrustedImm32

[Saam Barati]

Comment on attachment 294215 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h:112
> +        case GetArgument:

Please make this rule more specific.

[Yusuke Suzuki]

Comment on attachment 294215 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:60
>> +                summary:            157.26 ms +- 5.96 ms
> 
> Nice!

Yeah!

>> Source/JavaScriptCore/builtins/ArrayPrototype.js:314
>> +    var relativeStart = @toInteger(@argument(1));
> 
> toInteger of undefined is zero?

Yes

>> Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h:112
>> +        case GetArgument:
> 
> Please make this rule more specific.

OK, fixed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6784
>> +    m_jit.load32(CCallHelpers::payloadFor(CallFrameSlot::argumentCount), argumentCountGPR);
> 
> This doesn't seem correct if we've been inlined. Can you add tests?

Yeah, fixed. And I've added tests for that.
Add a new helper function AssemblyHelpers::argumentCount(const CodeOrigin&).

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6785
>> +    auto argumentOutOfBounds = m_jit.branch32(CCallHelpers::LessThanOrEqual, argumentCountGPR, CCallHelpers::Imm32(node->argumentIndex()));
> 
> Nit: This could be TrustedImm32 since it comes from our builtins.

Nice, fixed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6791
> +#endif

And use AssemblyHelpers::argumentsStart for that.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5168
>> +        LValue argumentCount = m_out.load32(payloadFor(CallFrameSlot::argumentCount));
> 
> Ditto, this does't look correct for inlining (I guess varargs inlinled callee is when this could come up because of your bytecode parser rules)

Fixed.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5177
> +        ValueFromBlock inBoundsResult = m_out.anchor(m_out.load64(payloadFor(CallFrameSlot::thisArgument + m_node->argumentIndex())));

And use AssemblyHelpers::argumentsStart for that.

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:511
>> +    Jump argumentOutOfBounds = branch32(LessThanOrEqual, regT0, Imm32(index));
> 
> Ditto nit: Could be TrustedImm32

Fixed.

[Yusuke Suzuki]

Committed r208524: <http://trac.webkit.org/changeset/208524>