923028 - Optimize the spread operator

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andy Wingo [:wingo]]

Currently the spread operator is implemented with an opcode (JSOP_SPREAD).  Spread calls, evals, and the like do a JSOP_SPREAD to collect an array, then apply to that array.  Point being, currently everything goes through JSOP_SPREAD.

The problem is that now with for-of iteration, we bounce back to JS for the initial @@iterator call, and then again to call next() on the iterator, for each iteration.

Basically the way to speed this up is that instead of emitting JSOP_SPREAD, emit a loop like the one in yield* or for-of.  This will make the iteration calls transparent, and open up the whole thing to baseline and ion compilation.  (Currently JSOP_SPREAD isn't even baseline compiled.)

Comment 1

[Jason Orendorff [:jorendorff]]

I think this is the right way to proceed. Let's make sure though.

Comment 2

[Jan de Mooij [:jandem]]

Yeah this sounds great.

(In reply to Andy Wingo [:wingo] from comment #0)
> This will make the iteration
> calls transparent, and open up the whole thing to baseline and ion
> compilation.  (Currently JSOP_SPREAD isn't even baseline compiled.)

For [1, ...foo] we also have to compile JSOP_INITELEM_INC. That shouldn't be too hard though and looks like it's a separate issue right? (It would be nice if we could emit JSOP_INITELEM instead of JSOP_INITELEM_INC for elements before the spread operator, the index is still known in that case).

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> (It would be
> nice if we could emit JSOP_INITELEM instead of JSOP_INITELEM_INC for
> elements before the spread operator, the index is still known in that case).

Sorry that should be JSOP_INITELEM_ARRAY, not JSOP_INITELEM. JSOP_INITELEM is only used for things like {0:foo}.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Bug 923028 - Part1: Emit JSOP_INITELEM_ARRAY for elements before spread operator.

(Now JSOP_INITELEM_INC and JSOP_SPREAD are Baseline compiled, though)

Emit JSOP_INITELEM_ARRAY instead of JSOP_INITELEM_INC for element before spread,
and push 'index' value just before first spread.

Bytecode for [10, 20, ...a, 30] is following:

  newarray 3
  int8 10
  initelem_array 0 // use initelem_array for element before spread
  int8 20
  initelem_array 1
  int8 2           // push 'index' here
  getgname "a"
  spread
  int8 30
  initelem_inc     // use initelem_inc for element after spread
  pop
  endinit

jstests and jit-test(--tbpl) are passed on Mac OS X 10.9.3 64bit.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part2: Emit loop instead of JSOP_SPREAD.

Emit loop instead of JSOP_SPREAD.
This is a draft patch. It seems to work, but may be wrong in some points.

With the patch, emitting a loop instead of JSOP_SPREAD improves the performance
only if the operand is not a dense array, in Baseline execution.
In other case, slower than current JSOP_SPREAD.
So, replacing JSOP_SPREAD with loop will introduce performance regression.

Things could be different in Ion execution,
which needs bug 874580 to be fixed and JSOP_INITELEM_INC to be Ion compiled,
now I'm trying to calculate the performance for this.
But we still have the problem in Interpreter and Baseline execution.
(Script needs 1000 execution before Ion compile, right?)


Loop code in current patch is following (almost same as for..of):

  // pre-condition         ARR I OBJ

  dup                   // ARR I OBJ OBJ
  callprop "@@iterator" // ARR I OBJ @@ITERATOR
  swap                  // ARR I @@ITERATOR OBJ
  call 0                // ARR I ITER

  // move ITER down to the bottom
  pick 2                // I ITER ARR
  pick 2                // ITER ARR I

  goto JMP

 TOP:
  loophead              // ITER ARR I RESULT
  // store value into array
  getprop "value"       // ITER ARR I VALUE
  initelem_inc          // ITER ARR (I+1)

 JMP:
  loopentry 0           // ITER ARR I
  dupat 2               // ITER ARR I ITER

  dup                   // ITER ARR I ITER ITER
  callprop "next"       // ITER ARR I ITER NEXT
  swap                  // ITER ARR I NEXT ITER
  undefined             // ITER ARR I NEXT ITER UNDEFINED
  call 1                // ITER ARR I RESULT

  dup                   // ITER ARR I RESULT RESULT
  getprop "done"        // ITER ARR I RESULT DONE?
  ifeq TOP              // ITER ARR I RESULT

  // remove RESULT and ITER
  //   iter, arr, (index+count), result => arr, (index+count)
  pick 3                // ARR (I+COUNT) RESULT ITER
  popn 2                // ARR (I+COUNT)

jstests and jit-test(--tbpl) are passed on Mac OS X 10.9.3 64bit.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Performance comparison

Performance comparison is following:

[INITELEM]
  Code:
    a = []; let s = elapsed(); for (let i = 0; i < 1000000; i ++) [...a]; print(elapsed() - s);
    a = []; let s = elapsed(); for (let i = 0; i < 1000000; i ++) [1, ...a]; print(elapsed() - s);
    a = []; let s = elapsed(); for (let i = 0; i < 1000000; i ++) [1, 2, ...a]; print(elapsed() - s);
    ...
  Result:
    (please refer attached image)
    Interpreter: INITELEM_ARRAY is 1.9% faster than INITELEM_INC
    Baseline.    INITELEM_ARRAY is 3.5% faster than INITELEM_INC

    Performance improvement in both case.


[SPREAD Array]
  Code:
    a = []; let s = elapsed(); for (let i = 0; i < 200000; i ++) [...a]; print(elapsed() - s);
    a = [1]; let s = elapsed(); for (let i = 0; i < 200000; i ++) [...a]; print(elapsed() - s);
    a = [1, 2]; let s = elapsed(); for (let i = 0; i < 200000; i ++) [...a]; print(elapsed() - s);
    ...
  Result:
    Interpreter: JSOP_SPREAD is 1615% faster than loop
    Baseline:    JSOP_SPREAD is 298% faster than loop

    no performance improvement :(


[SPREAD Set]
  Code:
    a = new Set([]); let s = elapsed(); for (let i = 0; i < 50000; i ++) [...a]; print(elapsed() - s);
    a = new Set([1]); let s = elapsed(); for (let i = 0; i < 50000; i ++) [...a]; print(elapsed() - s);
    a = new Set([1, 2]); let s = elapsed(); for (let i = 0; i < 50000; i ++) [...a]; print(elapsed() - s);
    ...
  Result:
    Interpreter: JSOP_SPREAD is 13% faster than loop
    Baseline:    loop is 36% faster than JSOP_SPREAD

    Performance improvement only in Baseline.

Comment 7

[Tooru Fujisawa [:arai]]

bug 1015742 may be related to this bug.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8426975 [details] [diff] [review]
Bug 923028 - Part1: Emit JSOP_INITELEM_ARRAY for elements before spread operator.

Nice patch, looks great!

(I'll get to your question soon; traveling a bit today.)

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8426978 [details] [diff] [review]
Part2: Emit loop instead of JSOP_SPREAD.

Review of attachment 8426978 [details] [diff] [review]:
-----------------------------------------------------------------

I like this approach. Thanks for the detailed perf numbers!

As you said, it does regress performance for arrays (but improves perf in other cases), we can do more in Ion to optimize that and this will also benefit real for-of loops.

Jason, what do you think?

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8426978 [details] [diff] [review]
Part2: Emit loop instead of JSOP_SPREAD.

Review of attachment 8426978 [details] [diff] [review]:
-----------------------------------------------------------------

Yes please.

Comment 11

[Jan de Mooij [:jandem]]

Part 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d06b7c9cc5b

(Green Linux64 Try run: https://tbpl.mozilla.org/?tree=Try&rev=6f2067b4d7b5)

Thanks again!

Comment 12

[Jason Orendorff [:jorendorff]]

arai,

Swatinem is landing a patch in bug 1015742 that conflicts with this patch, but not really. It actually does a small part of what you're doing in the second patch.

His patch is ready to check in, and he needs it to proceed with the destructuring assignment work he's doing. I'm sorry for the inconvenience. Hoping it's minor.

Comment 13

[Arpad Borsos [:Swatinem]]

I also hope I don’t break things now that i pushed my patch on top of your part1.
Your patch looks nice btw. Will have to rebase mine in bug 933276 on top of your part2.
But I guess I will have to do a few iterations on that one either way.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d06b7c9cc5b

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part2: Emit loop instead of JSOP_SPREAD.

I'd like to borrow EmitIteratorNext function from Swatinem's patch in bug 933276, with extra |depth| argument, it's useful for my patch too :)
Is it okay?

If it's better to make a separated patch for EmitIteratorNext and land it before this patch, I'll prepare it.
In that case, it may be better to include EmitSpread function which just emits JSOP_SPREAD.
So, we can reduce future conflict between bug 933276 and this bug.

Green on Linux64 try: https://tbpl.mozilla.org/?tree=Try&rev=fb86f823c191

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Performance comparison (part2)

Performance regression happens only in Interpreter execution, and it's a littie now,
because ForOfIterator with dense array is no longer optimized with JSOP_SPREAD in trunk.

So I think we don't have to wait until Ion compliation of JSOP_INITELEM_INC any more, for performance reason.

Comment 17

[Arpad Borsos [:Swatinem]]

Comment on attachment 8435874 [details] [diff] [review]
Part2: Emit loop instead of JSOP_SPREAD.

Review of attachment 8435874 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good from my end.

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 8435874 [details] [diff] [review]
Part2: Emit loop instead of JSOP_SPREAD.

Review of attachment 8435874 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, glad we can remove JSOP_SPREAD and SpreadOperation. I'll forward this to Jason though as he's more familiar with JSOP_SPREAD.

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 8435874 [details] [diff] [review]
Part2: Emit loop instead of JSOP_SPREAD.

Review of attachment 8435874 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing r?. This is good stuff, but it'll require some work to avoid losing some important test coverage.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4401,5 @@
>      return true;
>  }
>  
> +/**
> + * EmitIteratorNext expects the iterator be already be on the stack.

"expects the iterator to be on the stack at the given depth (0 = top of stack)."

@@ +4411,5 @@
> +    if (depth == 0) {
> +        if (Emit1(cx, bce, JSOP_DUP) < 0)                      // ITER ITER
> +            return false;
> +    } else {
> +        ptrdiff_t off = EmitN(cx, bce, JSOP_DUPAT, 3);         // ITER ... ITER

Can you call EmitDupAt?

@@ +4425,5 @@
> +    if (Emit1(cx, bce, JSOP_SWAP) < 0)                         // ITER NEXT ITER
> +        return false;
> +    if (Emit1(cx, bce, JSOP_UNDEFINED) < 0)                    // ITER NEXT ITER UNDEFINED
> +        return false;
> +    if (EmitCall(cx, bce, JSOP_CALL, 1) < 0)                   // ITER RESULT

Pre-existing bug, in some cases at least, but the spec requires that the .next method be called with no arguments here, and it's observable. Please fix it and make sure we have tests for this.

@@ +6070,4 @@
>  }
>  
>  static bool
> +EmitSpread(ExclusiveContext *cx, BytecodeEmitter *bce)

Comment this function, as you did for EmitIteratorNext.

@@ +6071,5 @@
>  
>  static bool
> +EmitSpread(ExclusiveContext *cx, BytecodeEmitter *bce)
> +{
> +    if (Emit2(cx, bce, JSOP_PICK, (jsbytecode)2) < 2)          // I ITER ARR

It should be `< 0` not `< 2`.

@@ +6073,5 @@
> +EmitSpread(ExclusiveContext *cx, BytecodeEmitter *bce)
> +{
> +    if (Emit2(cx, bce, JSOP_PICK, (jsbytecode)2) < 2)          // I ITER ARR
> +        return false;
> +    if (Emit2(cx, bce, JSOP_PICK, (jsbytecode)2) < 2)          // ITER ARR I

here too

::: js/src/tests/ecma_6/Array/for_of_1.js
@@ -1,4 @@
>  // Test corner cases of for-of iteration over Arrays.
> -// The current spidermonky JSOP_SPREAD implementation for function calls
> -// with '...rest' arguments uses a ForOfIterator to extract values from
> -// the array, so we use that mechanism to test ForOfIterator here.

Note what this comment is saying.

The purpose of these tests is to test ForOfIterator. It still needs to be tested. But now that JSOP_SPREAD is going away, these tests will not exercise ForOfIterator anymore.

Find a way to keep ForOfIterator under test. At worst, you could create a new shell builtin function (in js/src/shell/js.cpp) that basically computes x => [...x], but using ForOfIterator, like SpreadOperation used to do. Then that builtin could be used instead of `f(...arr)` to test ForOfIterator.

But maybe there is a cleverer way to do it; I don't know.

In any case it's important to test that class because the DOM bindings use it.

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Part2: Emit loop instead of JSOP_SPREAD.

Thank you for reviewing!
Here is fixed patch.

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=25735363ef9d

(In reply to Jason Orendorff [:jorendorff] from comment #19)
> Pre-existing bug, in some cases at least, but the spec requires that the
> .next method be called with no arguments here, and it's observable. Please
> fix it and make sure we have tests for this.

js/src/jit-test/tests/for-of/next-arity.js tests it,
and it fails with the change, so I fixed the test too :)

No other regression is happened for the change in try run.

> ::: js/src/tests/ecma_6/Array/for_of_1.js
> @@ -1,4 @@
> >  // Test corner cases of for-of iteration over Arrays.
> > -// The current spidermonky JSOP_SPREAD implementation for function calls
> > -// with '...rest' arguments uses a ForOfIterator to extract values from
> > -// the array, so we use that mechanism to test ForOfIterator here.
> 
> Note what this comment is saying.
> 
> The purpose of these tests is to test ForOfIterator. It still needs to be
> tested. But now that JSOP_SPREAD is going away, these tests will not
> exercise ForOfIterator anymore.
> 
> Find a way to keep ForOfIterator under test. At worst, you could create a
> new shell builtin function (in js/src/shell/js.cpp) that basically computes
> x => [...x], but using ForOfIterator, like SpreadOperation used to do. Then
> that builtin could be used instead of `f(...arr)` to test ForOfIterator.
> 
> But maybe there is a cleverer way to do it; I don't know.
> 
> In any case it's important to test that class because the DOM bindings use
> it.

Sorry, I should read the comment carefully.

SetObject::construct method in js/src/builtin/MapObject.cpp uses ForOfIterator,
so I changed |f(...arr)| to |f(...new Set(arr))| in for_of_[1-4].js.
There are no duplicated entries in |arr|, and Set retains the order,
so those two expression behave just the same way.

Comment 21

[Jason Orendorff [:jorendorff]]

Comment on attachment 8435874 [details] [diff] [review]
Part2: Emit loop instead of JSOP_SPREAD.

Review of attachment 8435874 [details] [diff] [review]:
-----------------------------------------------------------------

On a second look, I see a lot of copy-and-paste coding in this patch, and we can do better:

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6083,5 @@
> +            outerLoop = LoopStmtInfo::fromStmtInfo(outer);
> +            break;
> +        }
> +    }
> +    uint32_t loopDepth = outerLoop ? outerLoop->loopDepth + 1 : 1;

It looks like this was copied out of PushLoopStatement. Instead, we need to pretend that [...x] is a loop statement, and actually call PushLoopStatement. That way, things like BytecodeEmitter::isInLoop() will return the correct result. We already do that for array comprehensions (EmitArrayComp -> EmitTree case PNK_FOR -> EmitFor -> EmitForOf -> PushLoopStatement).

In fact the way array comprehensions work is interesting and relevant; you might want to take a look at that code. Note that you can use the shell's parse() builtin to see very roughly what parse nodes look like:

    js> parse("[...y]")
    (STATEMENTLIST [(SEMI (ARRAY [(SPREAD y)]))])
    js> parse("[x for (x of y)]")
    (STATEMENTLIST [(SEMI (ARRAYCOMP [(LEXICALSCOPE (FOR (FOROF (VAR [x])
                                                                x
                                                                y)
                                                         (ARRAYPUSH x)))]))])

@@ +6125,5 @@
> +        return false;
> +
> +    ptrdiff_t beq = EmitJump(cx, bce, JSOP_IFEQ, top - bce->offset()); // ITER ARR I RESULT
> +    if (beq < 0)
> +        return false;

Again, this reads like a copy of code we already have in EmitForIn.

If possible, either generalize EmitForIn or factor out more common code. Perhaps EmitIteratorNext was not the right chunk of code to factor out, and we should have something more like this:

    if (!EmitForOfLoopStart(...))
        return false;
    if (!Emit1(cx, bce, JSOP_INITELEM_INC) < 0)
        return false;
    if (!EmitForOfLoopEnd(...))
        return false;


Thanks for the patch, arai. I'm looking forward to seeing JSOP_SPREAD deleted.

Comment 22

[Jason Orendorff [:jorendorff]]

(In reply to Tooru Fujisawa [:arai] from comment #20)
> SetObject::construct method in js/src/builtin/MapObject.cpp uses
> ForOfIterator,
> so I changed |f(...arr)| to |f(...new Set(arr))| in for_of_[1-4].js.
> There are no duplicated entries in |arr|, and Set retains the order,
> so those two expression behave just the same way.

And now it tests both mechanisms at once. Excellent! :)

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 8438227 [details] [diff] [review]
Part2: Emit loop instead of JSOP_SPREAD.

Review of attachment 8438227 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing r?. See comment 21.

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: Part2: Emit loop instead of JSOP_SPREAD.

Thank you for letting me know parse() function, it's very useful! :)

Okay, I generalized EmitForOf, by adding extra parameter |type|,
which should be STMT_FOR_OF_LOOP or STMT_SPREAD (new statement type for spread).
EmitIteratorNext was removed for now.

Then, I have some questions.

1. I didn't modify parse tree for spread, so |...y| stays |(SPREAD y)|, and
   EmitForOf is called directly from EmitSpread, by passing null as parse node,
   is it okay?
   or should I modify parse tree for spread to make it contain PNK_FOR node?

2. Which is better to remove EmitSpread function or leave it for abstraction?
   (Now it just calls EmitForOf, with extra parameters.)

3. It sesms that we don't need |update| field in StmtInfoBCE struct for spread.
   (it's used by |continue| statement, but |continue| statement never appear
    inside spread, right?)
   The value is set to |top| parameter in EmitForOf, and seems to be set to
   correct value later.
   What is the appropriate |top| value for spread's case? (-1 is used in this patch)
   Should I set |update| to correct value later? (I don't set it in this patch)

Try run is green: https://tbpl.mozilla.org/?tree=Try&rev=9251b323a37f

Comment 25

[Jason Orendorff [:jorendorff]]

(In reply to Tooru Fujisawa [:arai] from comment #24)
> 1. I didn't modify parse tree for spread, so |...y| stays |(SPREAD y)|, and
>    EmitForOf is called directly from EmitSpread, by passing null as parse
> node,
>    is it okay?

Yes.

> 2. Which is better to remove EmitSpread function or leave it for abstraction?
>    (Now it just calls EmitForOf, with extra parameters.)

Mmm, either one is fine with me.

> 3. It sesms that we don't need |update| field in StmtInfoBCE struct for spread.
>    (it's used by |continue| statement, but |continue| statement never appear
>     inside spread, right?)
>    The value is set to |top| parameter in EmitForOf, and seems to be set to
>    correct value later.
>    What is the appropriate |top| value for spread's case? (-1 is used in
> this patch)
>    Should I set |update| to correct value later? (I don't set it in this
> patch)

You don't need to set it, but make sure you MOZ_ASSERT when it *is* used that it was set!

Please rebase this patch and set r?me!

Comment 26

[Tooru Fujisawa [:arai]]

Attachment: 0001-Bug-923028-Part2-Emit-loop-instead-of-JSOP_SPREAD.patch

Thanks!
Here is updated patch.
Nothing is changed from last one except rebasing.

(In reply to Jason Orendorff [:jorendorff] from comment #25)
> > 2. Which is better to remove EmitSpread function or leave it for abstraction?
> >    (Now it just calls EmitForOf, with extra parameters.)
> 
> Mmm, either one is fine with me.

Okay, I left it.

> > 3. It sesms that we don't need |update| field in StmtInfoBCE struct for spread.
> >    (it's used by |continue| statement, but |continue| statement never appear
> >     inside spread, right?)
> >    The value is set to |top| parameter in EmitForOf, and seems to be set to
> >    correct value later.
> >    What is the appropriate |top| value for spread's case? (-1 is used in
> > this patch)
> >    Should I set |update| to correct value later? (I don't set it in this
> > patch)
> 
> You don't need to set it, but make sure you MOZ_ASSERT when it *is* used
> that it was set!

Currently the value -1 is not used anywhere (just a default value, like -1 for breaks/continues),
so MOZ_ASSERT is not needed, right?

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=f9b93c0b94db

Comment 27

[Jason Orendorff [:jorendorff]]

Comment on attachment 8447629 [details] [diff] [review]
0001-Bug-923028-Part2-Emit-loop-instead-of-JSOP_SPREAD.patch

Review of attachment 8447629 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments below addressed.


I wonder if the bit in PushLoopStatement that goes

    int loopSlots;
    if (type == STMT_FOR_OF_LOOP)
        loopSlots = 2;
    else if (type == STMT_FOR_IN_LOOP)
        loopSlots = 1;
    else
        loopSlots = 0;

needs to be updated for type == STMT_SPREAD. I think the existing code happens to mean that we will never OSR into Ion in an array-spread loop. But maybe the code should be more explicit, or assert, or have a comment. jandem, are you willing to tell arai what he should do here?

Separately: please add a case to the switch statement in NonLocalExitScope::prepareForNonLocalJump that just says

      case STMT_SPREAD:
        MOZ_ASSERT_UNREACHABLE("can't break/continue/return from inside a spread");

(It's really just a comment. Assertions are the best comments--well, I guess good types are a bit better.)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4518,5 @@
>      if (EmitLoopHead(cx, bce, nullptr) < 0)
>          return false;
>  
> +    if (type == STMT_SPREAD)
> +        bce->stackDepth ++;

Style nit: no space between stackDepth and ++.

@@ +4576,1 @@
>      if (Emit1(cx, bce, JSOP_DUP) < 0)                          // ITER ITER ITER

The comments are confusing now that we have two different possible stack layouts. In particular this last comment "// ITER ITER ITER" is only correct when type == STMT_FOR_OF_LOOP.

Please see if you can find a way to make the comments more accurate, perhaps like this:
>    if (Emit1(cx, bce, JSOP_DUP) < 0) // ... ITER ITER
or like this:
>    if (Emit1(cx, bce, JSOP_DUP) < 0) // (ITER or ITER ARR I) ITER ITER

@@ +6141,5 @@
> +{
> +    if (!EmitForOf(cx, bce, STMT_SPREAD, nullptr, -1))
> +        return false;
> +
> +    return true;

Simplify to `return EmitForOf(...);`.

Comment 28

[Jan de Mooij [:jandem]]

Comment on attachment 8447629 [details] [diff] [review]
0001-Bug-923028-Part2-Emit-loop-instead-of-JSOP_SPREAD.patch

Review of attachment 8447629 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jason Orendorff [:jorendorff] from comment #27)
> I wonder if the bit in PushLoopStatement that goes
> 
>     int loopSlots;
>     if (type == STMT_FOR_OF_LOOP)
>         loopSlots = 2;
>     else if (type == STMT_FOR_IN_LOOP)
>         loopSlots = 1;
>     else
>         loopSlots = 0;
> 
> needs to be updated for type == STMT_SPREAD. I think the existing code
> happens to mean that we will never OSR into Ion in an array-spread loop. But
> maybe the code should be more explicit, or assert, or have a comment.
> jandem, are you willing to tell arai what he should do here?

If it's true that STMT_SPREAD has 3 values on the stack instead of the 2 values for STMT_FOR_OF_LOOP, you could add:

if (type == STMT_SPREAD)
    loopSlots = 3;
else if ...

To this if-else.

And let's add the following assert after the if-else:

MOZ_ASSERT(loopSlots >= stmt->stackDepth);

Let me know if you run into problems with that.

Comment 29

[Tooru Fujisawa [:arai]]

Attachment: Bug 923028 - Part2: Emit loop instead of JSOP_SPREAD.

(In reply to Jason Orendorff [:jorendorff] from comment #27)
> @@ +4576,1 @@
> >      if (Emit1(cx, bce, JSOP_DUP) < 0)                          // ITER ITER ITER
> 
> The comments are confusing now that we have two different possible stack
> layouts. In particular this last comment "// ITER ITER ITER" is only correct
> when type == STMT_FOR_OF_LOOP.
> 
> Please see if you can find a way to make the comments more accurate, perhaps
> like this:
> >    if (Emit1(cx, bce, JSOP_DUP) < 0) // ... ITER ITER
> or like this:
> >    if (Emit1(cx, bce, JSOP_DUP) < 0) // (ITER or ITER ARR I) ITER ITER

Okay, I use "..." style.

(In reply to Jan de Mooij [:jandem] from comment #28)
> If it's true that STMT_SPREAD has 3 values on the stack instead of the 2
> values for STMT_FOR_OF_LOOP, you could add:
> 
> if (type == STMT_SPREAD)
>     loopSlots = 3;
> else if ...
> 
> To this if-else.
> 
> And let's add the following assert after the if-else:
> 
> MOZ_ASSERT(loopSlots >= stmt->stackDepth);
> 
> Let me know if you run into problems with that.

Thanks, I added them.


Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=7186f8fc6581

Comment 30

[Jan de Mooij [:jandem]]

Comment on attachment 8451330 [details] [diff] [review]
Bug 923028 - Part2: Emit loop instead of JSOP_SPREAD.

Review of attachment 8451330 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the PushLoopStatement changes, let me know if there are other parts I should review as well.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +720,2 @@
>      else
>          loopSlots = 0;

Uber nit: maybe add the new case as the first one, so that the "list" is still sorted (3, 2, 1, 0). Either way is fine with me though.

Comment 31

[Jason Orendorff [:jorendorff]]

Comment on attachment 8451330 [details] [diff] [review]
Bug 923028 - Part2: Emit loop instead of JSOP_SPREAD.

Review of attachment 8451330 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 32

[Tooru Fujisawa [:arai]]

Attachment: Bug 923028 - Part2: Emit loop instead of JSOP_SPREAD.

Thank you for reviewing!

(In reply to Jan de Mooij [:jandem] from comment #30)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +720,2 @@
> >      else
> >          loopSlots = 0;
> 
> Uber nit: maybe add the new case as the first one, so that the "list" is
> still sorted (3, 2, 1, 0). Either way is fine with me though.

Fixed :)

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=adc5e5e19509

Comment 33

[Jan de Mooij [:jandem]]

Comment on attachment 8452206 [details] [diff] [review]
Bug 923028 - Part2: Emit loop instead of JSOP_SPREAD.

Review of attachment 8452206 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! (Next time if a patch has r+ with some comments/nits you can just fix them without asking for another review :)

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bece62562e23

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bece62562e23