Bug 574130 (harmony:spreadarray)

Prototype Harmony's spread operator for arrays in SpiderMonkey

RESOLVED FIXED in mozilla16



9 years ago
5 years ago


(Reporter: brendan, Assigned: Benjamin)


(Blocks: 1 bug, {dev-doc-complete})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [DocArea=JS], URL)


(1 attachment, 2 obsolete attachments)



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



9 years ago
Alias: harmony:spread

Comment 1

9 years ago
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!



8 years ago
Assignee: general → brendan


7 years ago
Assignee: brendan → bpeterson

Comment 2

7 years ago
Attachment #626236 - Flags: feedback?(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:

      getlocal a

      getlocal b

      getlocal c

      getlocal d
    arrayspread 6

it looked like this:

      int 0
      getlocal a

      int 1
      getlocal b

      getlocal c

      getlocal d

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.
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.
Attachment #626236 - Flags: feedback?(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 for (x of y)]
>- 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")]

  (a bug; it should print ONE then TWO)
...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]


7 years ago
Depends on: 761410

Comment 8

7 years ago
Posted patch array implementation (obsolete) — Splinter Review
Attachment #626236 - Attachment is obsolete: true
Attachment #630031 - Flags: review?(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
    [_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

::: 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 @@
>  }

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".
Attachment #630031 - Flags: review?(jorendorff) → review+

Comment 10

7 years ago
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.
Attachment #630031 - Attachment is obsolete: true


7 years ago
Attachment #630399 - Flags: review?(jorendorff)
Attachment #630399 - Flags: review?(jorendorff) → review+

Comment 11

7 years ago
It's ready to go. We'll get a better C++ for of API in bug 762285.
Keywords: checkin-needed
(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.
Benjamin, please file a new bug for the f(...x) syntax (and any other places where ... can be used).


7 years ago
Alias: harmony:spread → harmony:spreadarray
Keywords: dev-doc-needed
Summary: Prototype Harmony's spread operator in SpiderMonkey → Prototype Harmony's spread operator for arrays in SpiderMonkey


7 years ago
Keywords: checkin-needed

Comment 15

7 years ago
See bug 762363.
(Please set the target milestone when landing on inbound)

Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Depends on: 842884
You need to log in before you can comment on or make changes to this bug.