Bug 38408

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.

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

Created attachment 264908 [details] WIP it's a start

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.

Created attachment 264980 [details] WIP

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.

Created attachment 265726 [details] WIP almost done. I just have to fix a default parameter bug before landing this.

Created attachment 265823 [details] patch

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 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 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 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.

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.

... 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.

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.

Created attachment 265880 [details] patch with callOperation inside DFG.

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

Created attachment 265883 [details] patch renaming.

Created attachment 265891 [details] patch build fix.

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 on attachment 265891 [details] patch r=me

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

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