574130 - (harmony:spreadarray) Prototype Harmony's spread operator for arrays in SpiderMonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

See the bug URL. We want tracing, jaegering, decompilation, as usual. Spread in array initialisers too.

/be

Comment 1

[Brendan Eich [:brendan]]

This time (ahem, unlike proxies -- which got a pass perhaps because they're used by wrappers and other clients, but maybe that's no excuse), use jsversion.h and a JS_HAS_SPREAD macro, like the old ones there (retire some, move minimum compile-time JSVERSION forward).

Same for harmony:restparams.

We can leave these config'ed off in Firefox 4 to reduce risk to the codebase, but we shouldn't hide them from users, including in final releases, out of fear of changing them during standardization later. IMHO!

/be

Comment 2

[:Benjamin Peterson]

Attachment: draft implementation for array initializers

Comment 3

[Jason Orendorff [:jorendorff]]

OK. I just chatted with dvander about this, and it would be easier for the jit if the only magic thing about [a, b, ...c, d] is the ...c part, so instead of:

    newarray
      getlocal a

      getlocal b

      spreadmagic
      getlocal c
      undefined

      getlocal d
    arrayspread 6
    endinit

it looked like this:

    newarray
      int 0
      getlocal a
      initelem

      int 1
      getlocal b
      initelem

      getlocal c
      arraypushall

      getlocal d
      arraypush
    endinit

Note that this is more like how array literals already look. More opcodes, but that helps all the analyses and makes for leaner, faster jitcode.

We already have JSOP_ARRAYPUSH; we emit it in array comprehensions. JSOP_ARRAYPUSHALL would be the new magic.

dvander says it'd be even better to expand the loop inline, but it's not necessary now. Easy enough to do that part later.

Comment 4

[Jason Orendorff [:jorendorff]]

Another nice thing about JSOP_ARRAYPUSHALL is that its implementation can use js::ForOf to start with, and changing it later (once TC39 decides) won't affect bytecode.

Comment 5

[Jason Orendorff [:jorendorff]]

Things to test:

- evaluation order in [a(), ...b(), c()]
- In the shell there's a wrap(obj) function;
  test [...wrap(obj)] does the same thing as [...obj]
- [...[x for (x of y)]]
  [...(x for (x of y))]
- Trailing commas and "Elision"s:
  [...x,]  // equivalent to [...x]
  [...x,,] // [...x, <hole>]
  [, ...x] // [<hole>, ...x]
- Syntax errors:
  (1 ... n)
  [1 ... n]
  (...x)
  [...x for (x of y)]
  [...]
  [...,]

Comment 6

[Jason Orendorff [:jorendorff]]

>- evaluation order in [a(), ...b(), c()]

In particular, b()'s iterator() method should be called before c(). Even if TC39 decides to use the .length protocol here, the order of evaluation is observable:

  js> [...{get length() { print("ONE"); return 0; }}, print("TWO")]
  TWO
  ONE

  (a bug; it should print ONE then TWO)

Comment 7

[Jason Orendorff [:jorendorff]]

...in fact, you can observe it using only arrays:

  js> var a = [1, 2, 3];
  js> [...a, a.pop()]
  [1, 2, 3]  // should be [1, 2, 3, 3]

Comment 8

[:Benjamin Peterson]

Attachment: array implementation

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 630031 [details] [diff] [review]
array implementation

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

Looks much better! Just a few comments. I would still like to look over the final draft.

::: js/src/frontend/Parser.cpp
@@ +6745,5 @@
>               * the JSOP_GETLOCAL and JSOP_SETLOCAL ops. These ops have an
>               * immediate operand, the local slot's stack index from fp->spbase.
>               *
>               * The array comprehension iteration step, array.push(i * j) in
>               * the example above, is done by <i * j>; JSOP_ARRAYCOMP <array>,

While you're in the neighborhood, please correct this typo.
It should say JSOP_ARRAYPUSH not COMP.

::: js/src/jsfriendapi.h
@@ +477,5 @@
>  #define JSITER_KEYVALUE   0x4   /* destructuring for-in wants [key, value] */
>  #define JSITER_OWNONLY    0x8   /* iterate over obj's own properties only */
>  #define JSITER_HIDDEN     0x10  /* also enumerate non-enumerable properties */
>  #define JSITER_FOR_OF     0x20  /* harmony for-of loop */
> +#define JSITER_NULL_EMPTY 0x40  /* null and undefined should be treated as empty iterator */

Blah! New flag! Don't you know how hard it is to get rid of these?! ;)

It seems unlikely that TC39 will treat null and undefined differently depending on whether you write
    [...x]
or
    [_v for (_v of x)]

So why not just change JSITER_FOR_OF's behavior? Preferably in another bug, since they're separable concerns. Maybe after running it by dherman.

::: js/src/jsinfer.cpp
@@ +3857,3 @@
>                  } else {
> +                    // Iterator could put arbitrary things into the array.
> +                    types->addType(cx, Type::UnknownType());

Nit: I think this reads a little better the other way around ("else if (op == JSOP_SPREAD)").

@@ +3862,5 @@
>          } else {
>              pushed[0].addType(cx, Type::UnknownType());
>          }
> +        switch (op) {
> +        case JSOP_SPREAD:

Style nit: indent case-labels and "default:" two spaces, per
https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style

::: js/src/jsinterp.cpp
@@ +3270,5 @@
>              goto error;
>      }
> +    if (op == JSOP_INITELEM_INC) {
> +        JS_ASSERT(obj->isArray());
> +        regs.sp[-2].setInt32(JSID_TO_INT(id) + 1);

Overflow check here.

@@ +3279,3 @@
>  }
>  END_CASE(JSOP_INITELEM)
> +END_CASE(JSOP_INITELEM_INC)

Don't add another END_CASE. Elsewhere we only use one; the second one is just going to expand to a bit of unreachable code.

@@ +3289,5 @@
> +    while (true) {
> +        Value rval;
> +        bool onward;
> +        if (!IteratorMore(cx, iterator, &onward, &rval))
> +            goto error;

DRY suggests using js::ForOf (defined in jsiter.h) rather than duplicating its code.

It looks like JSOP_ITER/JSOP_ENDITER must be used with "try notes" in order to make sure the iterator is closed as we exit the loop due to an exception. (I don't think it makes any observable difference here, but it is hard to be sure since JSOP_ITER is an extension point for C++-based proxies, and there are all sorts of proxies out there.) Using js::ForOf would eliminate the need to worry about that.

@@ +3297,5 @@
> +        if (!IteratorNext(cx, iterator, &item))
> +            goto error;
> +        RootedId id(cx, INT_TO_JSID(count));
> +        if (!arr->defineGeneric(cx, id, item, NULL, NULL, JSPROP_ENUMERATE))
> +            goto error;

In JSOP_INITELEM, we have no guarantee that the index is really an integer (partly due to E4X), but here we do know and so we can use defineElement rather than defineGeneric. It'll be slightly faster, and we won't have to root a jsid.

@@ +3298,5 @@
> +            goto error;
> +        RootedId id(cx, INT_TO_JSID(count));
> +        if (!arr->defineGeneric(cx, id, item, NULL, NULL, JSPROP_ENUMERATE))
> +            goto error;
> +        count++;

Overflow check here too.

::: js/src/jsopcode.tbl
@@ +223,5 @@
>  OPDEF(JSOP_NEWOBJECT, 91, "newobject",  NULL,         5,  0,  1, 19,  JOF_OBJECT|JOF_TYPESET)
>  OPDEF(JSOP_ENDINIT,   92, "endinit",    NULL,         1,  0,  0, 19,  JOF_BYTE)
>  OPDEF(JSOP_INITPROP,  93, "initprop",   NULL,         5,  2,  1,  3,  JOF_ATOM|JOF_PROP|JOF_SET|JOF_DETECTING)
>  OPDEF(JSOP_INITELEM,  94, "initelem",   NULL,         1,  3,  1,  3,  JOF_BYTE|JOF_ELEM|JOF_SET|JOF_DETECTING)
> +OPDEF(JSOP_INITELEM_INC,95, "initelemincr", NULL,     1,  3,  2,  3,  JOF_BYTE|JOF_ELEM|JOF_SET)

"initelemincr" should be "initelem_inc".

Comment 10

[:Benjamin Peterson]

Attachment: address review comments

I can't say ForOf is much of an improvement. I had to engage in some horribleness to get the context needed in the operation. Perhaps my addled brain was just forgot the proper C++ way to do this.

Comment 11

[:Benjamin Peterson]

It's ready to go. We'll get a better C++ for of API in bug 762285.

Comment 12

[Jason Orendorff [:jorendorff]]

(In reply to Benjamin Peterson from comment #10)
> I can't say ForOf is much of an improvement. I had to engage in some
> horribleness to get the context needed in the operation.

Yeah. I didn't expect ForOf to be prettier, but it's more correct, with less duplication of tricky code.

I'll push this.

Comment 13

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34476c720f8f

Comment 14

[Jason Orendorff [:jorendorff]]

Benjamin, please file a new bug for the f(...x) syntax (and any other places where ... can be used).

Comment 15

[:Benjamin Peterson]

See bug 762363.

Comment 16

[Ed Morley [:emorley]]

(Please set the target milestone when landing on inbound)

https://hg.mozilla.org/mozilla-central/rev/34476c720f8f

Comment 17

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator