Bug 164582 - [JSC] Default parameters should be retrieved by op_get_argument opcode instead of changing arity
Summary: [JSC] Default parameters should be retrieved by op_get_argument opcode instea...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-09 23:46 PST by Yusuke Suzuki
Modified: 2017-03-16 06:52 PDT (History)
8 users (show)

See Also:


Attachments
Patch (34.84 KB, patch)
2017-03-12 10:36 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (44.06 KB, patch)
2017-03-12 23:08 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2016-11-09 23:46:46 PST
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.
Comment 1 Yusuke Suzuki 2017-03-12 10:36:05 PDT
Created attachment 304196 [details]
Patch

WIP
Comment 2 Yusuke Suzuki 2017-03-12 10:47:37 PDT
Note that this patch improves six-speed default.es6 by 4.05x.
Comment 3 Saam Barati 2017-03-12 11:30:22 PDT
(In reply to comment #2)
> Note that this patch improves six-speed default.es6 by 4.05x.

nice
Comment 4 Yusuke Suzuki 2017-03-12 23:08:03 PDT
Created attachment 304228 [details]
Patch
Comment 5 Saam Barati 2017-03-15 16:24:45 PDT
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 6 Yusuke Suzuki 2017-03-15 22:00:39 PDT
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.
Comment 7 Yusuke Suzuki 2017-03-15 22:13:02 PDT
Committed r214029: <http://trac.webkit.org/changeset/214029>
Comment 8 Yusuke Suzuki 2017-03-16 06:06:38 PDT
I'll soon fix the failures in debug build.
Comment 9 Yusuke Suzuki 2017-03-16 06:24:57 PDT
Committed r214040: <http://trac.webkit.org/changeset/214040>
Comment 10 Yusuke Suzuki 2017-03-16 06:52:53 PDT
Committed r214041: <http://trac.webkit.org/changeset/214041>