We should be able optimize the pattern where we spread a function's rest parameter to another call

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

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.

[Saam Barati]

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]

Created attachment 294736 [details]
WIP

Some more things, it now kind of works with a new type of ForwardVarargs.

[Saam Barati]

Created attachment 294911 [details]
WIP

It might be able to run SampleBench/Air now. Still have quite a few things left.

[Saam Barati]

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]

Created attachment 295016 [details]
WIP

OSR exit is starting to work.

[Saam Barati]

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]

Created attachment 295122 [details]
WIP

passes almost all JSC tests. Just need to write some more specific testing for this.

[Saam Barati]

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]

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]

Created attachment 295476 [details]
patch

[WebKit Commit Bot]

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]

*** Bug 164806 has been marked as a duplicate of this bug. ***

[Filip Pizlo]

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]

(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]

Created attachment 295693 [details]
patch for landing

[Saam Barati]

Created attachment 295694 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 295694 [details]
patch for landing

Clearing flags on attachment: 295694

Committed r209121: <http://trac.webkit.org/changeset/209121>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.