Bug 574130 (harmony:spreadarray)

Prototype Harmony's spread operator for arrays in SpiderMonkey

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: brendan, Assigned: Benjamin)

Tracking

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

Trunk
mozilla16
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

/be
(Reporter)

Updated

7 years ago
Alias: harmony:spread
(Reporter)

Comment 1

7 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!

/be
(Reporter)

Updated

6 years ago
Assignee: general → brendan
Blocks: 694100
(Assignee)

Updated

5 years ago
Assignee: brendan → bpeterson
(Assignee)

Comment 2

5 years ago
Created attachment 626236 [details] [diff] [review]
draft implementation for array initializers
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:

    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.
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)
  [...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")]
  TWO
  ONE

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

Updated

5 years ago
Depends on: 761410
(Assignee)

Comment 8

5 years ago
Created attachment 630031 [details] [diff] [review]
array implementation
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
    [...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".
Attachment #630031 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 630399 [details] [diff] [review]
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.
Attachment #630031 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 11

5 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/34476c720f8f
Benjamin, please file a new bug for the f(...x) syntax (and any other places where ... can be used).
(Assignee)

Updated

5 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
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/34476c720f8f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Depends on: 842884
Whiteboard: [DocArea=JS]
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.