Bug 163865 - We should be able optimize the pattern where we spread a function's rest parameter to another call
Summary: We should be able optimize the pattern where we spread a function's rest para...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
: 164806 (view as bug list)
Depends on: 163925 164258
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-23 02:34 PDT by Saam Barati
Modified: 2016-11-29 22:25 PST (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 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.
Comment 1 Saam Barati 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);
}
```
Comment 2 Saam Barati 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.
Comment 3 Saam Barati 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.
Comment 4 Saam Barati 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.
Comment 5 Saam Barati 2016-11-16 19:07:58 PST
Created attachment 295016 [details]
WIP

OSR exit is starting to work.
Comment 6 Saam Barati 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.
Comment 7 Saam Barati 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.
Comment 8 Saam Barati 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.
Comment 9 WebKit Commit Bot 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.
Comment 10 Saam Barati 2016-11-27 16:50:22 PST
Created attachment 295476 [details]
patch
Comment 11 WebKit Commit Bot 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.
Comment 12 Saam Barati 2016-11-28 12:48:36 PST
*** Bug 164806 has been marked as a duplicate of this bug. ***
Comment 13 Filip Pizlo 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).
Comment 14 Filip Pizlo 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.
Comment 15 Saam Barati 2016-11-29 19:30:06 PST
Created attachment 295693 [details]
patch for landing
Comment 16 Saam Barati 2016-11-29 19:35:08 PST
Created attachment 295694 [details]
patch for landing
Comment 17 WebKit Commit Bot 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>
Comment 18 WebKit Commit Bot 2016-11-29 22:25:10 PST
All reviewed patches have been landed.  Closing bug.