WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
163865
We should be able optimize the pattern where we spread a function's rest parameter to another call
https://bugs.webkit.org/show_bug.cgi?id=163865
Summary
We should be able optimize the pattern where we spread a function's rest para...
Saam Barati
Reported
2016-10-23 02:34:28 PDT
The most trivial of cases would be eliminating two array allocations and the array iterator protocol loop in something like this: ``` function foo(...args) { return bar(...args); } ``` ES6SampleBench/Air has code similar to this: ``` visitArg(index, func, ...args) { let replacement = func(this._args[index], ...args); if (replacement) this._args[index] = replacement; } ``` Since this function is never called with more than 3 arguments, I rewrote the code like this to see what the performance improvement ballpark is: ``` visitArg(index, func, a1, a2, a3) { let replacement = func(this._args[index], a1, a2, a3); if (replacement) this._args[index] = replacement; } ``` This made the benchmark speed up by ~50%. I'm guessing we should be able to make this benchmark at least 30-45% faster by optimizing this type of pattern in the DFG/FTL.
Attachments
WIP
(23.49 KB, patch)
2016-11-12 11:19 PST
,
Saam Barati
no flags
Details
Formatted Diff
Diff
WIP
(37.54 KB, patch)
2016-11-14 13:35 PST
,
Saam Barati
no flags
Details
Formatted Diff
Diff
WIP
(81.84 KB, patch)
2016-11-15 18:54 PST
,
Saam Barati
no flags
Details
Formatted Diff
Diff
WIP
(82.83 KB, patch)
2016-11-16 16:20 PST
,
Saam Barati
no flags
Details
Formatted Diff
Diff
WIP
(96.00 KB, patch)
2016-11-16 19:07 PST
,
Saam Barati
no flags
Details
Formatted Diff
Diff
WIP
(106.17 KB, patch)
2016-11-17 00:14 PST
,
Saam Barati
no flags
Details
Formatted Diff
Diff
WIP
(105.50 KB, patch)
2016-11-17 19:23 PST
,
Saam Barati
no flags
Details
Formatted Diff
Diff
patch
(109.15 KB, patch)
2016-11-21 19:20 PST
,
Saam Barati
no flags
Details
Formatted Diff
Diff
patch
(121.99 KB, patch)
2016-11-27 16:50 PST
,
Saam Barati
fpizlo
: review+
Details
Formatted Diff
Diff
patch for landing
(121.72 KB, patch)
2016-11-29 19:30 PST
,
Saam Barati
no flags
Details
Formatted Diff
Diff
patch for landing
(121.69 KB, patch)
2016-11-29 19:35 PST
,
Saam Barati
no flags
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Saam Barati
Comment 1
2016-11-12 11:19:13 PST
Created
attachment 294622
[details]
WIP It beings. It can run this program and do the proper optimization: ``` function assert(b) { if (!b) throw new Error; } function baz(a, b, c, d) { return [a, b, c, d]; } function bar(a, b, ...args) { return baz(a, b, ...args); } function foo(a, b, c, d) { return bar(a, b, c, d); } noInline(foo); for (let i = 0; i < 10000; i++) { let [a, b, c, d] = foo(i, i+1, i+2, i+3); assert(a === i); assert(b === i+1); assert(c === i+2); assert(d === i+3); } ```
Saam Barati
Comment 2
2016-11-14 13:35:42 PST
Created
attachment 294736
[details]
WIP Some more things, it now kind of works with a new type of ForwardVarargs.
Saam Barati
Comment 3
2016-11-15 18:54:26 PST
Created
attachment 294911
[details]
WIP It might be able to run SampleBench/Air now. Still have quite a few things left.
Saam Barati
Comment 4
2016-11-16 16:20:57 PST
Created
attachment 294996
[details]
WIP We can forward varargs from a spread, and we can call varargs with spread. I still need to do OSR exit and make sure all the things work. I also want to make NewArrayWithSpread understand how to create itself from PhantomSpreads even if the NewArrayWithSpread itself is escaped.
Saam Barati
Comment 5
2016-11-16 19:07:58 PST
Created
attachment 295016
[details]
WIP OSR exit is starting to work.
Saam Barati
Comment 6
2016-11-17 00:14:54 PST
Created
attachment 295037
[details]
WIP NewArrayWithSpread now can accept arbitrary PhantomSpreads as arguments even if it itself must be allocated. PhantomSpread can only become Phantom if its argument is CreateRest and the CreateRest doesn't escape. I think this is probably needed for correctness because we don't know if any other incoming array has arbitrary indexed getters on it. If we want to PhantomSpread arbitrary arrays, we probably need a way to prove that the act of spreading wouldn't call into user observable code.
Saam Barati
Comment 7
2016-11-17 19:23:37 PST
Created
attachment 295122
[details]
WIP passes almost all JSC tests. Just need to write some more specific testing for this.
Saam Barati
Comment 8
2016-11-21 19:20:00 PST
Created
attachment 295319
[details]
patch I think this patch is just about ready, I just need to write a changelog and a few more tests.
WebKit Commit Bot
Comment 9
2016-11-21 19:22:43 PST
Attachment 295319
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:994: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
Saam Barati
Comment 10
2016-11-27 16:50:22 PST
Created
attachment 295476
[details]
patch
WebKit Commit Bot
Comment 11
2016-11-27 16:52:27 PST
Attachment 295476
[details]
did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:13: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:19: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:49: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:50: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:67: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:68: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:69: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:979: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 8 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.
Saam Barati
Comment 12
2016-11-28 12:48:36 PST
***
Bug 164806
has been marked as a duplicate of this bug. ***
Filip Pizlo
Comment 13
2016-11-29 16:48:31 PST
Comment on
attachment 295476
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=295476&action=review
> Source/JavaScriptCore/b3/B3ValueRep.h:81 > + // It's not valid output representation.
*a valid
> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4331 > if (m_graph.isWatchingHavingABadTimeWatchpoint(m_node)) {
I don't know from this method name if it means that we are having a bad time or if the watchpoint set is valid (i.e. we are not having a bad time).
Filip Pizlo
Comment 14
2016-11-29 16:51:53 PST
(In reply to
comment #13
)
> Comment on
attachment 295476
[details]
> patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=295476&action=review
> > > Source/JavaScriptCore/b3/B3ValueRep.h:81 > > + // It's not valid output representation. > > *a valid > > > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4331 > > if (m_graph.isWatchingHavingABadTimeWatchpoint(m_node)) { > > I don't know from this method name if it means that we are having a bad time > or if the watchpoint set is valid (i.e. we are not having a bad time).
Never mind, I was reading this wrong. I didn't parse the "isWatching" right. Please disregard.
Saam Barati
Comment 15
2016-11-29 19:30:06 PST
Created
attachment 295693
[details]
patch for landing
Saam Barati
Comment 16
2016-11-29 19:35:08 PST
Created
attachment 295694
[details]
patch for landing
WebKit Commit Bot
Comment 17
2016-11-29 22:25:05 PST
Comment on
attachment 295694
[details]
patch for landing Clearing flags on attachment: 295694 Committed
r209121
: <
http://trac.webkit.org/changeset/209121
>
WebKit Commit Bot
Comment 18
2016-11-29 22:25:10 PST
All reviewed patches have been landed. Closing bug.
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