Bug 38408 – [ES6] Add support for rest parameters

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Erik Arvidsson]

Rest parameters (aka var args aka rest args) are one of the agreed features for the next version of ECMAScript (Harmony)

function f(a, ...b) {
  assertTrue(b instanceof Array);
  return b.length;
}

assertEquals(0, f());
assertEquals(0, f(1));
assertEquals(1, f(1, 2));
assertEquals(3, f(1, 2, 3, 4));

Rest parameters is one of the carrots towards getting rid of "arguments".

[Oliver Hunt]

We'll hold off on this until there's less flux in the spec.

I believe strict mode has more value than this as well.

Otoh, anything that avoids using the arguments object == win.

[Mark Lam]

*** Bug 147041 has been marked as a duplicate of this bug. ***

[Saam Barati]

Created attachment 264908 [details]
WIP

it's a start

[Saam Barati]

Created attachment 264950 [details]
WIP

Change op_create_rest_parameter to op_fill_rest_parameter_array so we can let normal array allocation profiling kick in with op_new_array.

[Saam Barati]

Created attachment 264980 [details]
WIP

[Saam Barati]

Created attachment 265714 [details]
WIP

Almost done.
I need some more tests, specifically syntax tests.
I want to write a few more optimizations to have us
only do a C call when we have more things to fill than
the argument length.

[Saam Barati]

Created attachment 265726 [details]
WIP

almost done.
I just have to fix a default parameter bug before landing this.

[Saam Barati]

Created attachment 265823 [details]
patch

[WebKit Commit Bot]

Attachment 265823 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5362:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5363:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5364:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5365:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5366:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 265823 [details]
patch

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

Great!

> Source/JavaScriptCore/ChangeLog:13
> +        that the op_fill_rest_parameter_array opcode does not have a result.

OK, so, op_fill_rest_parameter_array's def becomes 0 :)

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:319
> +    if (needsArguments && !codeBlock->isStrictMode() && !parameters.hasDefaultParameterValues() && !m_restParameter) {

Is the functionality (aliasing parameters variables <=> arguments' elements) disabled when using rest parameters?
If so, adding a comment with spec URL is good for readers :)

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5365
> +    m_jit.setupArgument(1, [&] (GPRReg destGPR) { m_jit.move(arrayGPR, destGPR); });

Is this `arrayGPR` safe? Is there any case that `arrayGPR` aliases an argument register that is already filled (and broken)?

> Source/JavaScriptCore/parser/Parser.cpp:712
> +        declareRestOrNormalParameter(name, duplicateIdentifier);

Nice clean up.

> Source/JavaScriptCore/parser/Parser.cpp:1537
> +            failIfTrue(match(COMMA), "Rest parameter should be the last parameter in a function declaration"); // Let's have a good error message for this common case.

Nice for good syntax error message :D

[Saam Barati]

Comment on attachment 265823 [details]
patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:319
>> +    if (needsArguments && !codeBlock->isStrictMode() && !parameters.hasDefaultParameterValues() && !m_restParameter) {
> 
> Is the functionality (aliasing parameters variables <=> arguments' elements) disabled when using rest parameters?
> If so, adding a comment with spec URL is good for readers :)

It is disabled. It's a good idea to add a link to the spec, I'll do that.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5365
>> +    m_jit.setupArgument(1, [&] (GPRReg destGPR) { m_jit.move(arrayGPR, destGPR); });
> 
> Is this `arrayGPR` safe? Is there any case that `arrayGPR` aliases an argument register that is already filled (and broken)?

I didn't consider this. It seems like you're right and that this might not be safe.
I'll just go ahead and use our callOperation(...) mechanism.

[Geoffrey Garen]

Comment on attachment 265823 [details]
patch

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

I'll say r- since we don't think the call operation is correct. But please consider just using an inline loop.

> Source/JavaScriptCore/bytecode/BytecodeList.json:132
> +            { "name" : "op_fill_rest_parameter_array", "length": 3 }

"op_fill_rest_parameter_array" is a bit long for readability in bytecode dumps.

How about op_copy_rest? I believe that "rest" is a term of art for parameters now, and not ambiguous with other stuff in the language.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5369
> +    silentSpillAllRegisters(argumentsLengthGPR);
> +    // Arguments: 0:exec, 1:JSCell* array, 2:arguments start, 3:number of arguments to skip, 4:number of arguments or size_t max.
> +    m_jit.setupArgument(4, [&] (GPRReg destGPR) { m_jit.move(argumentsLengthGPR, destGPR); });
> +    m_jit.setupArgument(3, [&] (GPRReg destGPR) { m_jit.move(TrustedImm32(node->numberOfArgumentsToSkip()), destGPR); });
> +    m_jit.setupArgument(2, [&] (GPRReg destGPR) { emitGetArgumentStart(node->origin.semantic, destGPR); });
> +    m_jit.setupArgument(1, [&] (GPRReg destGPR) { m_jit.move(arrayGPR, destGPR); });
> +    m_jit.setupArgument(0, [&] (GPRReg destGPR) { m_jit.move(GPRInfo::callFrameRegister, destGPR); });
> +    appendCall(operationFillRestParameterArray);
> +    silentFillAllRegisters(argumentsLengthGPR);
> +    m_jit.exceptionCheck();

As far as I can tell, rest parameter copying is not dynamic and should never fail. Can we just compile this into an inline loop with no slow path?

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3567
> +            m_out.operation(operationFillRestParameterArray), m_callFrame, lowCell(m_node->child1()),

Ditto.

[Geoffrey Garen]

To elaborate on the "inline loop" suggestion: If you emitted DFG nodes for looping over arguments and storing into the array, then, in the common case, object allocation sinking would totally eliminate the overhead of allocating an array and copying into it. This is a very powerful optimization.

[Geoffrey Garen]

... But even if the allocation could not be eliminated, copying one or two arguments inline in the common case will be faster than making a function call.

[Geoffrey Garen]

Thinking about this more, we'll probably get the most bang for our buck if we notice an inline rest call, since it exposes a constant number of parameters.

So, let's land this feature with the callOperation fixed, and then land an optimization in the DFG byte code parser to notice an inline rest call and turn it into an allocation of a fixed-sized array with individual direct stores to the array.

[Saam Barati]

Created attachment 265880 [details]
patch

with callOperation inside DFG.

[Saam Barati]

oops I forgot to rename.
I'm renaming to op_copy_rest byte code instruction and CopyRest DFG node.

[Saam Barati]

Created attachment 265883 [details]
patch

renaming.

[Saam Barati]

Created attachment 265891 [details]
patch

build fix.

[WebKit Commit Bot]

Attachment 265891 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3637:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 265891 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 265891 [details]
patch

Rejecting attachment 265891 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 265891, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ng file Source/JavaScriptCore/tests/stress/rest-parameter-basics.js
patching file Source/JavaScriptCore/tests/stress/rest-parameter-inlined.js
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/js/parser-syntax-check-expected.txt
patching file LayoutTests/js/script-tests/parser-syntax-check.js

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Geoffrey Garen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/451991

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/192671