Bug 38408

Summary: [ES6] Add support for rest parameters
Product: WebKit Reporter: Erik Arvidsson <arv>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, commit-queue, ggaren, keith_miller, mark.lam, m.goleb+bugzilla, msaboff, oliver, sbarati, ysuzuki
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://wiki.ecmascript.org/doku.php?id=harmony:rest_parameters
Bug Depends on:    
Bug Blocks: 80559, 151454    
Attachments:
Description Flags
WIP
none
WIP
none
WIP
none
WIP
none
WIP
none
patch
ggaren: review-
patch
none
patch
none
patch none

Description Erik Arvidsson 2010-04-30 15:47:27 PDT
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".
Comment 1 Oliver Hunt 2010-05-01 12:08:46 PDT
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.
Comment 2 Mark Lam 2015-09-10 10:17:28 PDT
*** Bug 147041 has been marked as a duplicate of this bug. ***
Comment 3 Saam Barati 2015-11-05 17:38:25 PST
Created attachment 264908 [details]
WIP

it's a start
Comment 4 Saam Barati 2015-11-06 12:24:44 PST
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.
Comment 5 Saam Barati 2015-11-06 16:51:28 PST
Created attachment 264980 [details]
WIP
Comment 6 Saam Barati 2015-11-17 15:53:45 PST
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.
Comment 7 Saam Barati 2015-11-17 18:29:22 PST
Created attachment 265726 [details]
WIP

almost done.
I just have to fix a default parameter bug before landing this.
Comment 8 Saam Barati 2015-11-18 18:33:32 PST
Created attachment 265823 [details]
patch
Comment 9 WebKit Commit Bot 2015-11-18 18:37:05 PST
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.
Comment 10 Yusuke Suzuki 2015-11-19 06:11:53 PST
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
Comment 11 Saam Barati 2015-11-19 10:42:32 PST
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.
Comment 12 Geoffrey Garen 2015-11-19 11:16:30 PST
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.
Comment 13 Geoffrey Garen 2015-11-19 11:17:50 PST
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.
Comment 14 Geoffrey Garen 2015-11-19 11:18:26 PST
... 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.
Comment 15 Geoffrey Garen 2015-11-19 11:53:56 PST
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.
Comment 16 Saam Barati 2015-11-19 12:07:19 PST
Created attachment 265880 [details]
patch

with callOperation inside DFG.
Comment 17 Saam Barati 2015-11-19 12:09:43 PST
oops I forgot to rename.
I'm renaming to op_copy_rest byte code instruction and CopyRest DFG node.
Comment 18 Saam Barati 2015-11-19 12:26:29 PST
Created attachment 265883 [details]
patch

renaming.
Comment 19 Saam Barati 2015-11-19 13:08:29 PST
Created attachment 265891 [details]
patch

build fix.
Comment 20 WebKit Commit Bot 2015-11-19 13:17:28 PST
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.
Comment 21 Geoffrey Garen 2015-11-19 16:05:00 PST
Comment on attachment 265891 [details]
patch

r=me
Comment 22 WebKit Commit Bot 2015-11-19 16:47:09 PST
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
Comment 23 Saam Barati 2015-11-19 18:38:14 PST
landed in:
http://trac.webkit.org/changeset/192671