WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
164582
[JSC] Default parameters should be retrieved by op_get_argument opcode instead of changing arity
https://bugs.webkit.org/show_bug.cgi?id=164582
Summary
[JSC] Default parameters should be retrieved by op_get_argument opcode instea...
Yusuke Suzuki
Reported
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.
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
saam
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1
2017-03-12 10:36:05 PDT
Created
attachment 304196
[details]
Patch WIP
Yusuke Suzuki
Comment 2
2017-03-12 10:47:37 PDT
Note that this patch improves six-speed default.es6 by 4.05x.
Saam Barati
Comment 3
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
Yusuke Suzuki
Comment 4
2017-03-12 23:08:03 PDT
Created
attachment 304228
[details]
Patch
Saam Barati
Comment 5
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|?
Yusuke Suzuki
Comment 6
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.
Yusuke Suzuki
Comment 7
2017-03-15 22:13:02 PDT
Committed
r214029
: <
http://trac.webkit.org/changeset/214029
>
Yusuke Suzuki
Comment 8
2017-03-16 06:06:38 PDT
I'll soon fix the failures in debug build.
Yusuke Suzuki
Comment 9
2017-03-16 06:24:57 PDT
Committed
r214040
: <
http://trac.webkit.org/changeset/214040
>
Yusuke Suzuki
Comment 10
2017-03-16 06:52:53 PDT
Committed
r214041
: <
http://trac.webkit.org/changeset/214041
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug