Bug 171057 – [JSC] Use JSFixedArray directly when using call_varargs

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

[Yusuke Suzuki]

Currently, when executing the following code,

var array = [0, 1, 2];
Math.max(...array);

The bytecode becomes like this.

[  49] spread            loc9, loc10
[  52] new_array_with_spread loc7, loc9, 1, BitVector:0:1
[  57] mov               loc9, loc8
[  60] call_varargs      loc5, loc5, loc8, loc7, -11, 0    predicting None

But (52)'s array is unnecessary if argument is only consisted by one spread.

[Yusuke Suzuki]

Created attachment 307595 [details]
Patch

WIP

[Saam Barati]

Comment on attachment 307595 [details]
Patch

I was thinking about doing something like this when I added JSFixedArray. I think this is probably best done as a transformation over DFG IR. If you can prove the array hasn't changed, you can pass the JSFixedArray directly in. That said, maybe it's worth setting up the infrastructure with array literals so it can be done in the bytecode generator. But I think the most success would be to do this over DFG IR

[Yusuke Suzuki]

(In reply to Saam Barati from comment #2)
> Comment on attachment 307595 [details]
> Patch
> 
> I was thinking about doing something like this when I added JSFixedArray. I
> think this is probably best done as a transformation over DFG IR. If you can
> prove the array hasn't changed, you can pass the JSFixedArray directly in.
> That said, maybe it's worth setting up the infrastructure with array
> literals so it can be done in the bytecode generator. But I think the most
> success would be to do this over DFG IR

Yeah, that's nice insight! Thanks, eliminating NewArrayWithSpread in (maybe arguments elimination phase or object allocation sinking phase) seems clean.
(And the current patch is lacking this phase change right now b/c of WIP :P)

But I'm also exploring this approach because it is beneficial in all the tiers while arguments elimination phase is only done in FTL.
And I'm also considering about CallVararg(RuntimeFunc, Spread).

Now, it's still rough design, I need to look into this deeply.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #3)
> Yeah, that's nice insight! Thanks, eliminating NewArrayWithSpread in (maybe
> arguments elimination phase or object allocation sinking phase) seems clean.
> (And the current patch is lacking this phase change right now b/c of WIP :P)
> 
> But I'm also exploring this approach because it is beneficial in all the
> tiers while arguments elimination phase is only done in FTL.
> And I'm also considering about CallVararg(RuntimeFunc, Spread).
> 
> Now, it's still rough design, I need to look into this deeply.

OK, I would like to do the both. By only emitting op_spread, we can eliminate the new_array_with_spread for call_varargs in all the tiers. And we also would like to relax the current limiation for NewArrayWithSpread in arguments elimination phase to accept non-phantom spread & non-create-rest-related spread.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #4)
> OK, I would like to do the both. By only emitting op_spread, we can
> eliminate the new_array_with_spread for call_varargs in all the tiers. And
> we also would like to relax the current limiation for NewArrayWithSpread in
> arguments elimination phase to accept non-phantom spread &
> non-create-rest-related spread.

Simple bytecompiler level one will optimize the case like.

call(...array)

It's super easy to do that, and it is super beneficial because it is effective even in LLInt. And the above call form is quite common, so I think it's worth doing in bytecompiler. And the code itself should be quite simple, just adding small code to BytecodeGenerator. (And support spread form varargs.)

But we would like to analyze dfg and optimize,

var array = [...values];
call(...array);

thing too. Such thing can be effectively done in DFG arguments elimination phase.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #5)
> (In reply to Yusuke Suzuki from comment #4)
> > OK, I would like to do the both. By only emitting op_spread, we can
> > eliminate the new_array_with_spread for call_varargs in all the tiers. And
> > we also would like to relax the current limiation for NewArrayWithSpread in
> > arguments elimination phase to accept non-phantom spread &
> > non-create-rest-related spread.
> 
> Simple bytecompiler level one will optimize the case like.
> 
> call(...array)
> 
> It's super easy to do that, and it is super beneficial because it is
> effective even in LLInt. And the above call form is quite common, so I think
> it's worth doing in bytecompiler. And the code itself should be quite
> simple, just adding small code to BytecodeGenerator. (And support spread
> form varargs.)
> 
> But we would like to analyze dfg and optimize,
> 
> var array = [...values];
> call(...array);
> 
> thing too. Such thing can be effectively done in DFG arguments elimination
> phase.

Yeah I agree. I think it's worth having this peephole optimization in the byte compiler. We can develop a more sophisticated analysis in the DFG later.

[Yusuke Suzuki]

Created attachment 307714 [details]
Patch

WIP

[Build Bot]

Comment on attachment 307714 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307717 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Created attachment 307908 [details]
Patch

[Saam Barati]

Comment on attachment 307908 [details]
Patch

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

nice. r=me.
I think once we implement a more sophisticated analysis to transform code into this, it'll be nice to have this support in the FTL.

> Source/JavaScriptCore/ChangeLog:12
> +        to oit new_array_with_spread. This is very simple and effective because this

typo: oit => emit

[Yusuke Suzuki]

Comment on attachment 307908 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:12
>> +        to oit new_array_with_spread. This is very simple and effective because this
> 
> typo: oit => emit

Ah, this is typo, we should fix it oit => omit. Thanks!

[Yusuke Suzuki]

Committed r215720: <http://trac.webkit.org/changeset/215720>