| Summary: | [JSC] Default parameters should be retrieved by op_get_argument opcode instead of changing arity | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||
| Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, fpizlo, ggaren, jfbastien, keith_miller, mark.lam, msaboff, saam | ||||||
| Priority: | P2 | ||||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
Created attachment 304196 [details]
Patch
WIP
Note that this patch improves six-speed default.es6 by 4.05x. (In reply to comment #2) > Note that this patch improves six-speed default.es6 by 4.05x. nice Created attachment 304228 [details]
Patch
Comment on attachment 304228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304228&action=review r=me > Source/JavaScriptCore/ChangeLog:21 > + The above is simple. However, it has the side effect that it always increase the arity of the function. > + While `function.length` does not increase, internally, the number of parameters of CodeBlock increases. > + This effectively prevent our DFG / FTL to perform inlining: currently we only allows DFG to inline > + the function with the arity less than or equal the number of passing arguments. It is OK. But when using > + default parameters, we frequently do not pass the argument for the parameter with the default value. > + Thus, in our current implementation, we frequently need to fixup the arity. And we frequently fail > + to inline the function. Great find. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1120 > + if (!nonSimpleArguments) > initializeNextParameter(); So our arity is just up to our first default-value parameter? > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3468 > + // If the parameter list is non simple one, it is handled in bindValue's code. This will be true for every parameter? > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1444 > + if (codeBlock->numParameters() > argumentCountIncludingThis) { numParameters() includes |this|? Comment on attachment 304228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304228&action=review Thanks! >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1120 >> initializeNextParameter(); > > So our arity is just up to our first default-value parameter? Yes. And it is the same to the ECMAScript's function.length. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3468 >> + // If the parameter list is non simple one, it is handled in bindValue's code. > > This will be true for every parameter? Yes. BytecodeGenerator::initializeDefaultParameterValuesAndSetupFunctionScopeStack does so. >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1444 >> + if (codeBlock->numParameters() > argumentCountIncludingThis) { > > numParameters() includes |this|? Yes. CodeBlock's numParameter includes |this| since it is part of arg from the point of virtual register view. Committed r214029: <http://trac.webkit.org/changeset/214029> I'll soon fix the failures in debug build. Committed r214040: <http://trac.webkit.org/changeset/214040> Committed r214041: <http://trac.webkit.org/changeset/214041> |
If you have a function like, function func(a, b, options = {}) { ... } `func.length` is 2. But the number of parameters of this function is counted as 3. So if you call it with the form `func(a, b)`, the arity check always fails. But in the above case, not passing options is common. Instead, we should retrieve the argument by using `op_get_argument`. It does not increase the arity.