for-of loops: Replace "iterator"-based customization with "@@iterator" (until ES6 symbols are implemented) and use ES6 iterator protocol (moving away from legacy SpiderMonkey iterator protocol)

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: bruant.d, Assigned: wingo)

Tracking

({addon-compat, dev-doc-complete, site-compat})

unspecified
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS])

Attachments

(2 attachments, 9 obsolete attachments)

(Reporter)

Description

6 years ago
Latest TC39 agreement [1] is use a symbol'ed property instead of the "iterator" (string) property.
The currently implemented (bug 725907) for-of customization protocol is based on "iterator". If content starts using the current non-standard extension... yada yada... de facto standard... blah blah.

As a conclusion, I believe it'd be better what's currently implemented was hidden from content. The for-of behavior would just be the default one (for objects as well as DOM collections).
People will be able to try customizing their for-of loops in beta/aurora/nightly or will have to wait for the standard way.


[1] https://mail.mozilla.org/pipermail/es-discuss/2012-July/024207.html
Jason, does this seem reasonable?
Flags: needinfo?(jorendorff)
Yes. I'm slightly in favor of hiding this extension.

I'm much less worried than David about content becoming dependent on it. It's Firefox-only.
Flags: needinfo?(jorendorff)
(Reporter)

Comment 3

6 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #2)
> Yes. I'm slightly in favor of hiding this extension.
> 
> I'm much less worried than David about content becoming dependent on it.
> It's Firefox-only.
Object.prototype.watch is Firefox-only too and apparently, you'll have to keep it forever (some form of it at least). See bug 903332 comment 21.
That comment was specifically about minor releases, like chemspills.
(Assignee)

Comment 5

6 years ago
We should take advantage of this opportunity to change the semantics as well, to make the @@iterator version implement the ES6 protocol (with {value:foo,done:bool} instead of StopIteration).
(Reporter)

Comment 6

6 years ago
(In reply to Andy Wingo from comment #5)
> We should take advantage of this opportunity to change the semantics as
> well, to make the @@iterator version implement the ES6 protocol (with
> {value:foo,done:bool} instead of StopIteration).
filed bug 907717 for that purpose as it can be done independently from the present bug.
(Assignee)

Comment 7

6 years ago
See bug 666396 also for yield* implications.
(Assignee)

Comment 8

6 years ago
We were talking about this in #jsapi.  The spec says (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-15.19.2) that iterator is exported by the std::iteration module, but we don't implement modules yet.  It doesn't say what @@iterator actually is.  This is a barrier to implementation, as without knowing what it is, we can't call it in for-of forms.

Jason was pretty tired with this situation so I suggested to just have it be the string '@@iterator' for now, and then when TC39 gets its act together we will change it to be the symbol or GUID or whatever is the current thing.

The attached patch implements this change, and changes for-of to use the new { value: foo, done: bool } protocol.  It doesn't update tests yet; there are some 150 failing jit-tests because for-of is now calling "@@iterator" on objects instead of "iterator".  Fixing it isn't as simple as just changing the names, because the protocol has to change too.
(Assignee)

Comment 9

6 years ago
Attachment #799660 - Flags: review?(jwalden+bmo)
Attachment #799660 - Flags: review?(jorendorff)
(Assignee)

Comment 10

6 years ago
A comment on the implementation.  It's like a for-in, but it keeps two values on the stack instead of one: the iterator, but also the result object.  That's because the loop check uses the result object to check for termination, but the loop head uses it also to get the value.  I tried having the value flow in directly and be popped off, but somehow that didn't interoperate correctly with IonBuilder's stack height simulations.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 907717
(Reporter)

Comment 12

6 years ago
(In reply to Andy Wingo from comment #8)
> We were talking about this in #jsapi.  The spec says
> (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-15.19.2) that
> iterator is exported by the std::iteration module, but we don't implement
> modules yet.  It doesn't say what @@iterator actually is.
It's a symbol [1] (which aren't implemented either) 

> This is a barrier to implementation, as without knowing what it is, we can't
> call it in for-of forms.
> 
> Jason was pretty tired with this situation
I can relate... es-discuss is a good place to ask for clarifications though ;-) I'll start a thread.

> so I suggested to just have it be
> the string '@@iterator' for now, and then when TC39 gets its act together we
> will change it to be the symbol or GUID or whatever is the current thing.
the '@@iterator' string is as bad as the 'iterator' string. It doesn't remove the risk of de facto standard (which is the original motivation for this bug)
To fix this bug, either the standard thing has to be implemented (but we're modules and symbols away from this being possible, so it sounds far-fetched) or not implement the custom iterator protocol at all.
What I mean for the latter is to make for-of work as it does today, but not customizable (not getting obj.['iterator'] before running the loop), at least not for web content.

[1] very last point of http://wiki.ecmascript.org/doku.php?id=harmony:iterators
(Reporter)

Comment 13

6 years ago
(In reply to David Bruant from comment #12)
> (In reply to Andy Wingo from comment #8)
> > We were talking about this in #jsapi.  The spec says
> > (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-15.19.2) that
> > iterator is exported by the std::iteration module, but we don't implement
> > modules yet.  It doesn't say what @@iterator actually is.
> It's a symbol [1] (which aren't implemented either)
In the latest draft [1], section 8.1.6.4 (Well-Known Symbols and Intrinsics):
"Within this specification a well-known symbol is referred to by using a notation of the form @@name, where
“name” is one of the values listed in Table 10."
Table 10 right below lists @@iterator.

[1] http://wiki.ecmascript.org/lib/exe/fetch.php?id=harmony%3Aspecification_drafts&cache=cache&media=harmony:working_draft_ecma-262_edition_6_08-23-13-nomarkup.pdf
(In reply to Andy Wingo from comment #8)
> The attached patch implements this change, and changes for-of to use the new
> { value: foo, done: bool } protocol.  It doesn't update tests yet; there are
> some 150 failing jit-tests because for-of is now calling "@@iterator" on
> objects instead of "iterator".

I'll take a quick look, but the tests have to pass for this to be worth reviewing.

> Fixing it isn't as simple as just changing
> the names, because the protocol has to change too.

To start, change the names of the existing iterator methods on Array, Map, Set, TypedArray, and Generator to "@@iterator", update them to the new protocol, and let's see how many tests are still broken.

You'll also need to change CodeGen.py:
  https://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py#1482

Legacy generators need to continue to work with for-of. (Programs that weren't coded directly to the protocol shouldn't break.) Legacy generators need an @@iterator method that supports the new protocol. Gross, but at least it'll be easy...

There are actually four protocols:

  * Legacy -- using StopIteration and .__iterator__ methods. This must stay working.
    Don't change any tests that use the legacy protocol.

  * Nouveau obsolete -- what for-of does right now, using obj.iterator() and
    StopIteration. This we are killing with fire. Tests that depend on details
    of this protocol should be changed.

  * Pseudo ES6 -- what you're implementing here, using obj["@@iterator"]().
    This is what we're shooting for right now.

  * ES6 draft -- exactly the same as pseudo ES6, except for the name of the method,
    which is a symbol instead of a string.

Let's go. Might as well find out now if we're doomed to keep the nouveau obsolete protocol forever.
Comment on attachment 799660 [details] [diff] [review]
For-of calls @@iterator on its RHS

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

Looks about right; I'll wait for the full patch to look closer.
Attachment #799660 - Flags: review?(jwalden+bmo)
Attachment #799660 - Flags: review?(jorendorff)
(Assignee)

Comment 16

6 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #14)
> To start, change the names of the existing iterator methods on Array, Map,
> Set, TypedArray, and Generator to "@@iterator", update them to the new
> protocol, and let's see how many tests are still broken.

What I did locally was add @@iterator as a property to things like arrays and such, which wraps the existing iterator in a shim.  For new ES6 objects like maps and sets, I changed the iterator implementation, but it's a lot of work to get everything done.  Currently about 50 jit-tests are failing, all the ones that test map iterators, set iterators, and for-of iterators.  It's quite a slog.  Will update the patch tomorrow morning, probably.

> Legacy generators need to continue to work with for-of.

Really?  My current patch (to be uploaded) does this with the shim, but I wouldn't think this matters: it's an obsolete feature combined with a new feature.

Anyway I think after all this we'll be ready when @@iterator becomes a symbol -- we'll just have to update the name of the thing and not the protocol.  What a mess.
(In reply to Andy Wingo from comment #16)
> > Legacy generators need to continue to work with for-of.
> 
> Really?  My current patch (to be uploaded) does this with the shim, but I
> wouldn't think this matters: it's an obsolete feature combined with a new
> feature.

Yes really. for-of isn't new anymore.

This works now. Let's keep it working; it's something like 20 lines of code.
(Assignee)

Comment 18

6 years ago
(Assignee)

Updated

6 years ago
Attachment #799660 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Assignee: general → wingo
(Assignee)

Comment 19

6 years ago
Comment on attachment 800825 [details] [diff] [review]
for-of calls @@iterator on iteratee v2

A humongous patch.
Attachment #800825 - Attachment description: for-of calls @@iterator on iteratee → for-of calls @@iterator on iteratee v2
Attachment #800825 - Flags: review?(jwalden+bmo)
Attachment #800825 - Flags: review?(jorendorff)
Review proceeding nicely here.  Expect it tomorrow, probably after bug 666396 as it appears to be underneath this, and I need to understand that to understand this, it looks like.
Comment on attachment 800825 [details] [diff] [review]
for-of calls @@iterator on iteratee v2

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

Enough different things to warrant an r-.  Overall looks pretty good, tho.

::: js/src/builtin/Iterator.js
@@ +9,5 @@
> +function LegacyIteratorNext(arg) {
> +    try {
> +        return { value: this.iter.next(arg), done: false };
> +    } catch (e) {
> +        if (e instanceof StopIteration)

This likely wants to be std_StopIteration, same for the other mentions.

@@ +26,5 @@
> +    }
> +}
> +
> +function LegacyIterator(iter) {
> +    std_Object_defineProperty(this, 'iter', { value: iter });

Maybe instead of "iter" we could name this something long and fruity like "temporary iter name until ES6 stabilizes, do not use"?  This just seems slightly tempting as-is.

@@ +30,5 @@
> +    std_Object_defineProperty(this, 'iter', { value: iter });
> +}
> +
> +function LegacyIteratorWithThrow(iter) {
> +    std_Object_defineProperty(this, 'iter', { value: iter });

This doesn't work, because the object literal there has the page-controlled Object.prototype on its prototype chain.  So doing this:

Object.prototype.set = 42;

would cause any call to this method to throw, although that's presumably not what was intended.  Same for LegacyIterator as well.

There are a bunch more literals in this file that need fixing in this regard, actually.  In Intl code, we've taken to doing std_Object_create(null), then setting properties on that.  Not the best solution ever, but it gets the job done, and we can optimize it later if necessary.

@@ +35,5 @@
> +}
> +
> +var LegacyIteratorsInitialized = {}
> +
> +function InitLegacyIterators() {

This is kinda gnarly, but oh well.

@@ +43,5 @@
> +
> +    var LegacyIteratorProto = std_Object_create(GetIteratorPrototype(), props);
> +    MakeConstructible(LegacyIterator, LegacyIteratorProto);
> +
> +    props.throw = { value: LegacyIteratorThrow, configurable: true, writable: true };

I'd prefer seeing these descriptors include enumerable, even if its absence will imply the correct value (once you use the std_Object_create(null) trick).

@@ +58,5 @@
> +    iter = ToObject(iter);
> +    if ('throw' in iter)
> +        return new LegacyIterator(iter);
> +    else
> +        return new LegacyIteratorWithThrow(iter);

Add a comment by this like /* Make sure the iterator exposes a "throw" property so that it plays well with the ES6 iteration protocol. */ or so.  Also, don't have else after a return.

@@ +66,5 @@
> +    return NewLegacyIterator(this);
> +}
> +
> +function ArrayIteratorLegacyShim() {
> +    return NewLegacyIterator(std_Array_iterator.call(this));

callFunction(std_Array_iterator, this)

Done this way, you get poisoning if someone changes what Array.prototype["@@iterator"].call refers to.

::: js/src/builtin/MapObject.cpp
@@ +977,4 @@
>          js_delete(range);
>          thisobj.setReservedSlot(RangeSlot, PrivateValue(NULL));
> +        rval.setUndefined();
> +        done = true;

You could do this this way:

  if (!range || range->empty()) {
    rval.setUndefined();
    js_delete(range);
    thisobj.setReservedSlot(RangeSlot, PrivateValue(NULL));
    done = true;
  } else {

to common up slightly.  (js_delete being null-safe.)  You'd have redundant slot-sets and js_delete if !range, but I think that's probably preferable to extra noise here.

@@ +994,5 @@
> +
> +            JSObject *pairobj = NewDenseCopiedArray(cx, 2, pair);
> +            if (!pairobj)
> +                return false;
> +            rval = ObjectValue(*pairobj);

rval.setObject(*pairobj)

@@ +1194,5 @@
> +            if (!iter.next(&pairVal, &done))
> +                return false;
> +            if (done)
> +                break;
> +            RootedObject pairobj(cx, js_ValueToNonNullObject(cx, pairVal));

This was pre-existing, but we should put pairobj outside the loop, to avoid the every-loop add/remove traffic.

And, hey, more pre-existing!  I don't think js_ValueToNonNullObject is the right semantics here.  Map(iterable, comparator) in the latest spec, step 12h, says to throw if Type(nextItem) isn't Object.  Doesn't this impermissibly pass { done: false, value: "foo" } through as if it were { done: false, value: ["f", "o"] }?

(We pause now for Two Minutes' Hate for the mess that is all SpiderMonkey's internal and JSAPI methods for converting a value to an object or possibly null, possibly handling primitives in different entirely un-memorable ways that require implementation-reading every time to remember semantics.)

@@ +1560,4 @@
>          js_delete(range);
>          thisobj.setReservedSlot(RangeSlot, PrivateValue(NULL));
> +        rval.setUndefined();
> +        done = true;

Same (!range || range->empty()) commoning.

@@ +1703,1 @@
>              AutoHashableValueRooter key(cx);

AutoHashableValueRooter probably should move out of this loop.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1060,5 @@
>          return false;
>  
>      Rooted<StaticBlockObject*> blockObj(cx, &pn->pn_objbox->object->as<StaticBlockObject>());
>  
> +    int extraSlots = op == JSOP_ENTERLET1 ? 1 : (op == JSOP_ENTERLET2 ? 2 : 0);

Multi-lining this would help readability:

int extraSlots = (op == JSOP_ENTERLET1)
                 ? 1
                 : (op == JSOP_ENTERLET2)
                 ? 2
                 : 0;

@@ +4230,5 @@
>  
>  static bool
>  EmitForIn(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn, ptrdiff_t top)
>  {
> +    bool isForOf = pn->pn_iflags == JSITER_FOR_OF;

Blah, this method should probably split in two at some point.

@@ +4304,5 @@
> +        if (Emit1(cx, bce, JSOP_SWAP) < 0) // @@ITERATOR OBJ
> +            return false;
> +        if (Emit1(cx, bce, JSOP_NOTEARG) < 0)
> +            return false;
> +        if (Emit3(cx, bce, JSOP_CALL, ARGC_HI(0), ARGC_LO(0)) < 0) // ITER

unsigned forOfArgc = 0;

@@ +4310,5 @@
> +        CheckTypeSet(cx, bce, JSOP_CALL);
> +        if (Emit1(cx, bce, JSOP_UNDEFINED) < 0) // ITER RESULT
> +            return false;
> +    } else {
> +        if (Emit2(cx, bce, JSOP_ITER, (uint8_t) pn->pn_iflags) < 0)

uint8_t()

@@ +4403,5 @@
> +        if (Emit1(cx, bce, JSOP_SWAP) < 0)                         // ITER NEXT ITER
> +            return false;
> +        if (Emit1(cx, bce, JSOP_NOTEARG) < 0)
> +            return false;
> +        if (Emit3(cx, bce, JSOP_CALL, ARGC_HI(0), ARGC_LO(0)) < 0) // ITER RESULT

unsigned forOfNextArgc;

This looks like another place that should be passing |undefined| as an argument.

::: js/src/jit-test/lib/std-iterator.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// FIXME: Import from std::iteration.
> +const std_iterator = '@@iterator';

I would, instead of using the wishy-washy assertDeepEq method (how deep?), add an assertIteratorResult(value, done) method here, and use it in all these tests.  I'd have it check .done and .value specifically, and probably check that Object.getOwnPropertyNames is limited to exactly those two methods.  This is messy/big enough to be followup-worthy, tho -- just make sure to hit all the files you're hitting here.

I would also add a separate assertIteratorResultPair(pair, done) method to handle [key, value] iterator result testing.  Same followup territory.

::: js/src/jit-test/tests/collections/Map-clear-iterators-1.js
@@ +10,2 @@
>  
>  m = Map([["a", 1], ["b", 2], ["c", 3], ["d", 4]]);

Same new Map(...)/Map(iterable, comparator) thing as in some other test here.

::: js/src/jit-test/tests/collections/Map-iterator-pairs-2.js
@@ +5,1 @@
>  var map = Map([['a', 1], ['b', 2]]);

Maybe I'm reading ES6 wrong.  But isn't the list-of-key/value-pairs form of Map creation limited strictly to |new Map|, with |Map| only interpreting its arguments as |(iterable, comparator)|?  (This would be followup-land, to be sure.)

::: js/src/jit-test/tests/collections/Map-iterators-3.js
@@ +9,3 @@
>  m.clear();
>  m.set("a", 1);
> +assertDeepEq(it.next(), { value: undefined, done: true });  // close the iterator

The added comment is wrong -- should be "already closed" or something.

::: js/src/jit-test/tests/collections/Set-iterator-proxies-2.js
@@ +2,5 @@
>  
>  load(libdir + "asserts.js");
> +load(libdir + "std-iterator.js");
> +
> +function assertCrossCompartmentIteratorResult(result, expectedValue, expectedDone) {

If I'm not mistaken, this function is unnecessary if an assertIteratorResult method gets added/used.  (More evidence for why assertDeepEq being a wishy-washy way to assert stuff here.)

::: js/src/jit-test/tests/for-of/array-iterator-surfaces-2.js
@@ +23,5 @@
>  }
>  
> +check([][std_iterator]());
> +check(Array.prototype[std_iterator].call({}));
> +check(Array.prototype[std_iterator].call(undefined));

Spec seems to say that Array.prototype.values calls ToObject(this), which throws for |undefined|.  So this test is wrong, and I guess our implementation of the method is wrong, too.  Fix and add a test.

::: js/src/jit-test/tests/for-of/semantics-08.js
@@ +1,1 @@
> +// A for-of loop exits if the iterator's .next method returns a result from another compartment.

This comment's totally bogus any more, remove it.

@@ +4,3 @@
>  
>  var g = newGlobal('new-compartment');
> +var it = g.eval("({ '@@iterator': function () { return this; }, " +

Please interpolate std_iterator here.

::: js/src/jsiter.cpp
@@ +749,5 @@
> +
> +    RootedId valueId(cx, AtomToId(cx->names().value));
> +    RootedValue valueTmp(cx, value);
> +    if (!JSObject::setGeneric(cx, obj, obj, valueId, &valueTmp, false))
> +        return NULL;

You want defineProperty, not setGeneric -- set invokes setters, looks at the prototype chain, etc.  Try this, and then this method won't work as expected:

Object.defineProperty(Object.prototype, "value", { set: function() { throw 42; } });

Same for the other set-call here too.

@@ +1342,5 @@
>      NULL                     /* construct   */
>  };
>  
> +bool
> +ForOfIterator::next(MutableHandleValue val, bool *done)

vp is a more common name for this sort of thing.

@@ +1345,5 @@
> +bool
> +ForOfIterator::next(MutableHandleValue val, bool *done)
> +{
> +    if (!iterator)
> +        return false;

With a separate init method, this check is unnecessary.

@@ +1351,5 @@
> +    if (!JSObject::getProperty(cx, iterator, iterator, cx->names().next, &method))
> +        return false;
> +    RootedValue vp(cx);
> +    if (!Invoke(cx, ObjectOrNullValue(iterator), method, 0, NULL, &vp))
> +        return false;

Aren't you supposed to be calling the next method with the argument |undefined|, per IteratorNext(keys)?  This should be tested.

It's preferable to use InvokeArgs rather than Invoke for calling functions, btw -- also makes it easier to root the arguments to the function call.

@@ +1354,5 @@
> +    if (!Invoke(cx, ObjectOrNullValue(iterator), method, 0, NULL, &vp))
> +        return false;
> +    RootedObject resultObj(cx, ToObject(cx, vp));
> +    if (!resultObj)
> +        return false;

This is also wrong in the same way as in the other patch -- it's supposed to throw a TypeError if the returned value isn't an object.  Test?

@@ +1360,5 @@
> +    if (!JSObject::getProperty(cx, resultObj, resultObj, cx->names().done, &doneVal))
> +        return false;
> +    *done = ToBoolean(doneVal);
> +    if (!JSObject::getProperty(cx, resultObj, resultObj, cx->names().value, val))
> +        return false;

This seems wrong -- I don't think you're supposed to get the .value if .done is truthy.  Test?

::: js/src/jsiter.h
@@ +231,5 @@
> +/*
> + * Create an object of the form { value: VALUE, done: DONE }.
> + */
> +extern JSObject *
> +js_IteratorResult(JSContext *cx, js::HandleValue value, bool done);

Put this inside namespace js, and name it CreateItrResultObject, to comport with the lovely spec naming.  Also add a /* ES6 20130905 25.4.3.4 CreateItrResultObject */ comment before this, or something like that.

@@ +272,4 @@
>      {
>          RootedValue iterv(cx, iterable);
> +        if (ValueToIterator(cx, JSITER_FOR_OF, &iterv))
> +            iterator = &iterv.get().toObject();

I think probably this should do the usual thing we do because constructors can't be fallible: add a separate bool init(HandleValue iterable) method, and have that do this stuff.

::: js/src/jsopcode.cpp
@@ +169,5 @@
>      if (cs.ndefs >= 0)
>          return cs.ndefs;
>  
>      uint32_t n = NumBlockSlots(script, pc);
> +    return op == JSOP_ENTERLET1 ? n + 1 : (op == JSOP_ENTERLET2 ? n + 2 : n);

I think we should break this up into

if (op == JSOP_ENTERLET1)
  return n + 1;
if (op == JSOP_ENTERLET2)
  return n + 2;
return n;

at this point.

::: js/src/vm/CommonPropertyNames.h
@@ +36,5 @@
>      macro(construct, construct, "construct") \
>      macro(constructor, constructor, "constructor") \
>      macro(currency, currency, "currency") \
>      macro(currencyDisplay, currencyDisplay, "currencyDisplay") \
> +    macro(std_iterator, std_iterator, "@@iterator") \

Not to bikeshed, but we might want to make this string even more clearly don't-usable by making it not appear to perhaps be something exotic that people may actually, permissibly use:

"@@iterator: temporary string, stand-in for as-yet-unspecified semantics, don't use!"

Bonus points for it being untypably long, such that people will *have* to digest its contents before using it.  ;-)

jorendorff?  :-)
Attachment #800825 - Flags: review?(jwalden+bmo) → review-
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #21)
> ::: js/src/jit-test/tests/collections/Map-clear-iterators-1.js
> @@ +10,2 @@
> >  
> >  m = Map([["a", 1], ["b", 2], ["c", 3], ["d", 4]]);
> 
> Same new Map(...)/Map(iterable, comparator) thing as in some other test here.

More specifically, calling Map as a function is specced to not work at all normally; rather it should throw with a TypeError. The language in the spec allows it to be called as a function *only* when being called using `super` from an instantiating subclass constructor (which we don't implement yet).

(although technically you can do `Map.call(Map[@@create]())`, but we don't implement that yet either)

> >  var map = Map([['a', 1], ['b', 2]]);
> 
> Maybe I'm reading ES6 wrong.  But isn't the list-of-key/value-pairs form of
> Map creation limited strictly to |new Map|, with |Map| only interpreting its
> arguments as |(iterable, comparator)|?

There's no difference in the arguments that `Map` vs. `new Map` accepts. The way the spec is written is kind of confusing, but `new Map(...argumentList)` essentially forwards to `Map.apply(Map[@@create](), argumentList)`. The whole purpose of this indirection is to set up the semantics for @@create, which is how subclassing builtins is implemented.

> >  var g = newGlobal('new-compartment');
> > +var it = g.eval("({ '@@iterator': function () { return this; }, " +
> 
> Please interpolate std_iterator here.

This will need a bit of refactoring when @@iterator is a Symbol, due to embedding as a string (everywhere where `std_iterator` is used can be ignorant of whether it's a string or a Symbol.
(Assignee)

Comment 23

6 years ago
Thank you both for the detailed review!  I'll get on this next week.
Comment on attachment 800825 [details] [diff] [review]
for-of calls @@iterator on iteratee v2

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

Clearing review flag for now.

Let's fix Map and add @@create in a different bug. New tests should use `new Map`.
Attachment #800825 - Flags: review?(jorendorff)
Does the patch gets []["@@iterator"]()'s prototype chain right, even though it's using a shim?

Waldo's right, that stuff is icky. Does that save a lot of work? If so I'm 100% for it. But file a follow-up bug to rip it out, and fix the native implementations to do the right thing.

Or if you prefer, I can go through and do a prerequisite patch, adding an abstraction layer of CreateItrResultObject function calls that all our native iterators call. It would implement the legacy/nouveau-obsolete StopIteration-based protocol, but the functions would be called in exactly the right places so that you could switch them to the ES6 protocol in this bug.
(In reply to Jason Orendorff [:jorendorff] from comment #25)
> Or if you prefer, I can go through and do a prerequisite patch, adding an
> abstraction layer of CreateItrResultObject function calls that all our
> native iterators call. It would implement the legacy/nouveau-obsolete
> StopIteration-based protocol, but the functions would be called in exactly
> the right places so that you could switch them to the ES6 protocol in this
> bug.

I like plans like this because you can do this stuff fast. No change in behavior, so no new tests or changes to existing tests needed.
(Assignee)

Comment 27

6 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #25)
> Does the patch gets []["@@iterator"]()'s prototype chain right, even though
> it's using a shim?

Yes.  There are existing tests that check this.

> Waldo's right, that stuff is icky. Does that save a lot of work? If so I'm
> 100% for it. But file a follow-up bug to rip it out, and fix the native
> implementations to do the right thing.
> 
> Or if you prefer, I can go through and do a prerequisite patch, adding an
> abstraction layer of CreateItrResultObject function calls that all our
> native iterators call. It would implement the legacy/nouveau-obsolete
> StopIteration-based protocol, but the functions would be called in exactly
> the right places so that you could switch them to the ES6 protocol in this
> bug.

I think this would slow me down FWIW.  It takes approximately forever to do a round trip on these patches and now that we have started and have already made good steps towards ES6, doing a shim that adapts things in the backwards direction sounds, well, backwards to me.
(Assignee)

Comment 28

6 years ago
Thanks again for this awesome review.  Inline comments below; skipped nits will be fixed in fresh patch to be uploaded.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #21)
> Comment on attachment 800825 [details] [diff] [review]
> for-of calls @@iterator on iteratee v2
> ::: js/src/builtin/Iterator.js
> @@ +26,5 @@
> > +    }
> > +}
> > +
> > +function LegacyIterator(iter) {
> > +    std_Object_defineProperty(this, 'iter', { value: iter });
> 
> Maybe instead of "iter" we could name this something long and fruity like
> "temporary iter name until ES6 stabilizes, do not use"?  This just seems
> slightly tempting as-is.

Even better, new patch uses a weak map.

> @@ +30,5 @@
> > +    std_Object_defineProperty(this, 'iter', { value: iter });
> > +}
> > +
> > +function LegacyIteratorWithThrow(iter) {
> > +    std_Object_defineProperty(this, 'iter', { value: iter });
> 
> This doesn't work, because the object literal there has the page-controlled
> Object.prototype on its prototype chain.

Wow, great catches here and in related things.  Thank you!

> @@ +58,5 @@
> > +    iter = ToObject(iter);
> > +    if ('throw' in iter)
> > +        return new LegacyIterator(iter);
> > +    else
> > +        return new LegacyIteratorWithThrow(iter);
> 
> Add a comment by this like /* Make sure the iterator exposes a "throw"
> property so that it plays well with the ES6 iteration protocol. */ or so. 
> Also, don't have else after a return.

Actually this was backwards :-(  I had wanted to put "throw" on the wrapper iff it was on the wrappee.  Will be fixed in the new patch; I think I should be able to avoid the conditional entirely by dispatching more precisely.

> (We pause now for Two Minutes' Hate for the mess that is all SpiderMonkey's
> internal and JSAPI methods for converting a value to an object or possibly
> null, possibly handling primitives in different entirely un-memorable ways
> that require implementation-reading every time to remember semantics.)

Preach it!

> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +1060,5 @@
> >          return false;
> >  
> >      Rooted<StaticBlockObject*> blockObj(cx, &pn->pn_objbox->object->as<StaticBlockObject>());
> >  
> > +    int extraSlots = op == JSOP_ENTERLET1 ? 1 : (op == JSOP_ENTERLET2 ? 2 : 0);
> 
> Multi-lining this would help readability:
> 
> int extraSlots = (op == JSOP_ENTERLET1)
>                  ? 1
>                  : (op == JSOP_ENTERLET2)
>                  ? 2
>                  : 0;

Really? :)  I agree the first was bad, but I don't see yours as being better.  Anyway...  Do you think ENTERLET2 makes sense?  I mean, I could change ENTERLET1 to be ENTERLETN and take an argument.  Maybe it doesn't matter though.

> @@ +4230,5 @@
> >  
> >  static bool
> >  EmitForIn(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn, ptrdiff_t top)
> >  {
> > +    bool isForOf = pn->pn_iflags == JSITER_FOR_OF;
> 
> Blah, this method should probably split in two at some point.

Maybe now is the point, actually.  I'll see how it looks.

> @@ +4403,5 @@
> > +        if (Emit1(cx, bce, JSOP_SWAP) < 0)                         // ITER NEXT ITER
> > +            return false;
> > +        if (Emit1(cx, bce, JSOP_NOTEARG) < 0)
> > +            return false;
> > +        if (Emit3(cx, bce, JSOP_CALL, ARGC_HI(0), ARGC_LO(0)) < 0) // ITER RESULT
> 
> This looks like another place that should be passing |undefined| as an
> argument.

I guess the spec does suggest this to be the case.  Good catch?

> ::: js/src/jit-test/lib/std-iterator.js
> @@ +2,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +// FIXME: Import from std::iteration.
> > +const std_iterator = '@@iterator';
> 
> I would, instead of using the wishy-washy assertDeepEq method (how deep?),
> add an assertIteratorResult(value, done) method here, and use it in all
> these tests.  I'd have it check .done and .value specifically, and probably
> check that Object.getOwnPropertyNames is limited to exactly those two
> methods.  This is messy/big enough to be followup-worthy, tho -- just make
> sure to hit all the files you're hitting here.

Actually this is a useful cleanup that I landed as part of bug 666396, and makes these test changes much more sensible and smaller.  I'll go ahead and fold it in if that's OK.

> ::: js/src/jsiter.cpp
> @@ +749,5 @@
> > +
> > +    RootedId valueId(cx, AtomToId(cx->names().value));
> > +    RootedValue valueTmp(cx, value);
> > +    if (!JSObject::setGeneric(cx, obj, obj, valueId, &valueTmp, false))
> > +        return NULL;
> 
> You want defineProperty, not setGeneric -- set invokes setters, looks at the
> prototype chain, etc.  Try this, and then this method won't work as expected:
> 
> Object.defineProperty(Object.prototype, "value", { set: function() { throw
> 42; } });
> 
> Same for the other set-call here too.

Exciting.  Thanks for the catch.

> @@ +1351,5 @@
> > +    if (!JSObject::getProperty(cx, iterator, iterator, cx->names().next, &method))
> > +        return false;
> > +    RootedValue vp(cx);
> > +    if (!Invoke(cx, ObjectOrNullValue(iterator), method, 0, NULL, &vp))
> > +        return false;
> 
> Aren't you supposed to be calling the next method with the argument
> |undefined|, per IteratorNext(keys)?  This should be tested.

OK.  FWIW I think this was an unintentional, insignificant change in the spec resulting from refactoring -- previous versions just called iter.next().  What-ever.
 
> It's preferable to use InvokeArgs rather than Invoke for calling functions,
> btw -- also makes it easier to root the arguments to the function call.

Neat, hadn't seen that.  Will do.

> @@ +1354,5 @@
> > +    if (!Invoke(cx, ObjectOrNullValue(iterator), method, 0, NULL, &vp))
> > +        return false;
> > +    RootedObject resultObj(cx, ToObject(cx, vp));
> > +    if (!resultObj)
> > +        return false;
> 
> This is also wrong in the same way as in the other patch -- it's supposed to
> throw a TypeError if the returned value isn't an object.  Test?

You're probably right.  Is a followup OK after https://bugs.ecmascript.org/show_bug.cgi?id=1901 resolves itself?

> @@ +1360,5 @@
> > +    if (!JSObject::getProperty(cx, resultObj, resultObj, cx->names().done, &doneVal))
> > +        return false;
> > +    *done = ToBoolean(doneVal);
> > +    if (!JSObject::getProperty(cx, resultObj, resultObj, cx->names().value, val))
> > +        return false;
> 
> This seems wrong -- I don't think you're supposed to get the .value if .done
> is truthy.  Test?

You're right.  So many tests :-((

> ::: js/src/jsiter.h
> @@ +231,5 @@
> > +/*
> > + * Create an object of the form { value: VALUE, done: DONE }.
> > + */
> > +extern JSObject *
> > +js_IteratorResult(JSContext *cx, js::HandleValue value, bool done);
> 
> Put this inside namespace js, and name it CreateItrResultObject, to comport
> with the lovely spec naming.  Also add a /* ES6 20130905 25.4.3.4
> CreateItrResultObject */ comment before this, or something like that.

Surely full words are better, no?  I'll make this change, but it seems like a bad name to me.

> ::: js/src/vm/CommonPropertyNames.h
> @@ +36,5 @@
> >      macro(construct, construct, "construct") \
> >      macro(constructor, constructor, "constructor") \
> >      macro(currency, currency, "currency") \
> >      macro(currencyDisplay, currencyDisplay, "currencyDisplay") \
> > +    macro(std_iterator, std_iterator, "@@iterator") \
> 
> Not to bikeshed, but we might want to make this string even more clearly
> don't-usable by making it not appear to perhaps be something exotic that
> people may actually, permissibly use:
> 
> "@@iterator: temporary string, stand-in for as-yet-unspecified semantics,
> don't use!"
> 
> Bonus points for it being untypably long, such that people will *have* to
> digest its contents before using it.  ;-)

Whatever color works for me :)

I'll try to adapt all the yield* tests that make sense to work with for-of, as they are similar.  Hopefully done today, otherwise tomorrow.  Thanks again for the detailed review.
(Assignee)

Comment 29

6 years ago
(Assignee)

Updated

6 years ago
Attachment #800825 - Attachment is obsolete: true
(Assignee)

Comment 30

6 years ago
Comment on attachment 807342 [details] [diff] [review]
for-of calls @@iterator on iteratee

Updated patch that (I believe, though it's possible I missed one or two) all comments.  I don't envy the task, but Jeff (and Jason if you like) please do take a look!
Attachment #807342 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 31

6 years ago
Pending: rename to CreateItrResultObject and adapt yield* tests.
(Assignee)

Comment 33

6 years ago
The try builds were on fire because of some interesting self-hosted bugs.  This patch references a number of intrinsics both via JSFunctionSpec methods referenced in C++ and lexically.  Some of the intrinsics are referenced by multiple JSFunctionSpecs.  For functionspecs, when the prototype is copied, the methods on the prototype get a shallow copy, and are fully cloned only on first call.  This is in contrast to GETINTRINSIC which copies and clones on first reference.  Additionally the lazy cloning of functions would intern the fresh clone into the intrinsicsholder, regardless of what was already there (!).  So you could get into a situation where an intrinsic runs, collects feedback on a GETINTRINSIC, and then later a prototype is lazily initialized, interning a fresh clone of that same function into the intrinsicsholder.  Chaos ensures in IonBuilder, which wants the type feedback to match the binding of the intrinsic.

I pushed a new try build that removes the interning step from jsapi.cc:JS_DefineFunctions.  This way GETINTRINSIC is the only thing that interns values.  This means that all lexical references to a name get the same value, but OTOH all references via JSFunctionSpec are unique.  This latter issue is bug 875433.  However this hack strictly improves the current situation -- the old state of things was busted wrt bug 875433 and also broke lexically-referenced intrinsics.

Good times?

https://tbpl.mozilla.org/?tree=Try&rev=c5de9136f713
Blocks: 918075
Depends on: 918823
(Assignee)

Comment 34

6 years ago
This is missing corresponding changes to dom/bindings/Codegen.py, to make DOM iterables bind @@iterator.
(Assignee)

Updated

6 years ago
Blocks: 919948
(Assignee)

Comment 35

6 years ago
This is the current state of the patch.  There are still a few failures that I am tracking down, unfortunately: https://tbpl.mozilla.org/?tree=Try&rev=864d9d9c373f
Attachment #807342 - Attachment is obsolete: true
Attachment #807342 - Flags: review?(jwalden+bmo)
Attachment #809084 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 36

6 years ago
OK, I'm a little stuck and need some suggestions before I do something stupid :)

The issue is that the DOM bindings generator isn't really wired up to deal with self-hosted functions, and @@iterator is now self-hosted.  OK, so I change Codegen.py to emit raw JSFunctionSpec structs; fine.  But then BindingUtils.cpp doesn't really deal with self-hosted functions either: see around line 967.  That explains what I think are the last Mochitest failures that I need to fix.

So my question: How do I create a self-hosted function from BindingUtils.cpp?  JS_DefineFunctions will do it, but that takes multiple functionspecs and also sets the functions as properties on an object, but in this case that's not what I want.

I could add arguments to JS_NewFunction.  I could make a new API.  I've prototyped a few of these but it goes a bit deep and I'd like someone with actual taste (i.e. not me ;) to weigh in.
(Assignee)

Comment 37

6 years ago
(Assignee)

Updated

6 years ago
Attachment #809084 - Attachment is obsolete: true
Attachment #809084 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 years ago
Depends on: 920433
No longer depends on: 918823
(Assignee)

Comment 38

6 years ago
Comment on attachment 809852 [details] [diff] [review]
for-of calls @@iterator on iteratee v5

Okey smokey!  Rebased on top of bug 920433, which should fix trybuild issues I was seeing.  https://tbpl.mozilla.org/?tree=Try&rev=b55a64491c78 is the running trybuild.  Note that it would also be OK to rework some of the self-hosted things to follow the approach I use the latest patch revision on bug 919948 (that I'll post right now).

:Waldo, if you would be so good as to take another look here, that would be delightful :)
Attachment #809852 - Attachment description: for-of calls @@iterator on iteratee → for-of calls @@iterator on iteratee v5
Attachment #809852 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 39

6 years ago
At this point we're looking good: https://tbpl.mozilla.org/?tree=Try&rev=3b84c3c272ab
The only remaining issue is in the addon SDK.  There are three places that build iterators there that are used in for-of, but they do so via setting the "iterator" property.  They fail after this patch because for-of will now call @@iterator and not "iterator".

Since the SDK should also work with stable FF, the right thing to do is to make the SDK iterators work with both new and old for-of versions.  Since they also use generators to implement their iterators, and we can't use the new function* because the SDK should work with old FF, the right thing to do is to shim.

To shim there are two options: shim in the SDK, or make the SDK iterator prototypes inherit from Iterator.prototype.  Doing the latter gets you an @@iterator shim for free, so it's probably the right thing.  Otherwise the SDK could detect the for-of @@iterator object using https://gist.github.com/andywingo/6712480, and shim using something like what we do in Iterator.js in this patch.

I was talking with :zer0 from the SDK team and he wanted to talk with his tech lead before getting back with a plan.

The rest of this patch can be fruitfully reviewed, lacking these changes, so please Waldo if you have time I would love to move on past this.
(Assignee)

Comment 40

6 years ago
(Assignee)

Updated

6 years ago
Attachment #809852 - Attachment is obsolete: true
Attachment #809852 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 years ago
Attachment #810531 - Attachment description: for-of calls @@iterator on iteratee → for-of calls @@iterator on iteratee v6
Attachment #810531 - Flags: review?(jwalden+bmo)
(In reply to Andy Wingo [:wingo] from comment #39)

> Since they also use generators to implement their iterators, and we can't use the
> new function* because the SDK should work with old FF, the right thing to do
> is to shim.

I missed that part: SDK doesn't need to work with old FF atm, basically the SDK is shipped with the browser, so the add-on will use the SDK that is in the running browser: any changes we made in SDK master, unless it has to be uplifted, needs to works only with m-c.

So if this is simplify the things, we can use `function*` too. Otherwise I think inherit from Iterator would be the right thing to do.
So as far as I understand we could just update SDK code to take advantage of the standard iterator API
only drawback is that we'll have to use a temporary string instead of a symbol & later in time update to
the standard symbol.

It would be nice if there was some reference we could use to avoid second update, for example if `Iterator.iteratorSymbol` contained temorary string and later was updated to symbol.

I think it would be even better, if we actually exposed that symbol shim in a standard way, by which I mean,
if we had `Symbol` global constructor that could throw until we implement symbols, but had well-known symbol properties as defined by ES6. That way we would just used that property and implemented standard
ES6 iterator interfaces.

If there are not expose `Iterator.iteratorSymbol` or `Symbol` as suggested above, still not a big deal.
In that case SDK can have it's variable that we'll contain temporary string and later will become a symbol.



I prefer not to go with inheritance from `Iterator.prototype` solution as sometimes we inherit from proxies and mix in iterator interface and that won't work.
On the second thought I don't think there's much value in exposing `Iterator.iteratorSymbol` other than new symbol change will be able to land without us fixing stuff. So unless there's other code that could benefit from this probably it's not worth it.

I'd suggest to give us a time to prepare r+ patch for SDK, so we could fix a tree right after this change lands.


ZER0, I'd suggest to write a patch that defines `iteratorSymbol` variable like one here:
https://gist.github.com/andywingo/6712480#comment-916130

And use that `iteratorSymbol` to implement standard ES6 iterator interface.

We should also submit a bug to remove `iteratorSymbol` hack once actual symbols are
available, so basically make it depend on Bug 645416.
Comment on attachment 810531 [details] [diff] [review]
for-of calls @@iterator on iteratee v6

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

You know that part of this patch?  That part is the best thing I have ever seen in a patch.  (You know what I mean.)

::: js/src/builtin/Iterator.js
@@ +5,5 @@
> +function IteratorIdentity() {
> +    return this;
> +}
> +
> +var LegacyIteratorWrapperMap = new WeakMap();

std_WeakMap, please.

::: js/src/builtin/MapObject.cpp
@@ +965,5 @@
>  MapIteratorObject::next_impl(JSContext *cx, CallArgs args)
>  {
>      MapIteratorObject &thisobj = args.thisv().toObject().as<MapIteratorObject>();
>      ValueMap::Range *range = thisobj.range();
> +    RootedValue rval(cx);

For niceness of reading, I'd name this |value| -- then you pass value/done into the create method, which reads really nicely.

@@ +985,5 @@
> +            break;
> +
> +          case MapObject::Entries: {
> +            Value pair[2] = { range->front().key.get(), range->front().value };
> +            AutoValueArray root(cx, pair, 2);

ArrayLength(pair) instead.

@@ +1193,5 @@
> +            if (!iter.next(&pairVal, &done))
> +                return false;
> +            if (done)
> +                break;
> +            // FIXME: Bug 918341.

Expand this into something like

  // FIXME: We're supposed to throw a TypeError if !pairVal.isObject(): bug 918341.

@@ +1473,5 @@
>      SetIteratorObject::finalize
>  };
>  
>  const JSFunctionSpec SetIteratorObject::methods[] = {
> +    JS_SELF_HOSTED_FN("@@iterator", "IteratorIdentity", 0, 0),

This just occurred to me at this point, but could you change all the "@@iterator" strings to js_std_iterator_str?  (Everything in CommonPropertyNames.h gets a js_##foo##_str const char[] to go with it.)  Then removal of the @@iterator name will cause every C++ use to turn into build bustage, when we do it.

@@ +1549,5 @@
>  SetIteratorObject::next_impl(JSContext *cx, CallArgs args)
>  {
>      SetIteratorObject &thisobj = args.thisv().toObject().as<SetIteratorObject>();
>      ValueSet::Range *range = thisobj.range();
> +    RootedValue rval(cx);

Same value-naming as above.

::: js/src/builtin/Utilities.js
@@ +79,5 @@
>  var std_Set_has = Set.prototype.has;
> +var std_iterator = '@@iterator'; // FIXME: Change to be a symbol.
> +var std_StopIteration = StopIteration;
> +var std_Map_iterator = Map()[std_iterator];
> +var std_Set_iterator = Set()[std_iterator];

Can't these be {Map,Set}.prototype[std_iterator]?

@@ +81,5 @@
> +var std_StopIteration = StopIteration;
> +var std_Map_iterator = Map()[std_iterator];
> +var std_Set_iterator = Set()[std_iterator];
> +var std_Map_iterator_next = Object.getPrototypeOf(Map()[std_iterator]()).next;
> +var std_Set_iterator_next = Object.getPrototypeOf(Set()[std_iterator]()).next;

It'd be nice if these could be spelled out without constructing anything as well, but I think the GeneratorPrototype not being a named thing anywhere means maybe that's not possible.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1067,5 @@
>      Rooted<StaticBlockObject*> blockObj(cx, &pn->pn_objbox->object->as<StaticBlockObject>());
>  
> +    int extraSlots = (op == JSOP_ENTERLET1)
> +                     ? 1
> +                     : (op == JSOP_ENTERLET2)

We have this sort of ternary-chaining a bunch of places in SpiderMonkey, so its readability has probably tended to "grow" on the average SpiderMonkey hacker some, beyond what a random developer might think of it.  :-)  (I agree it starts to fall apart eventually, but I don't quite think this is to that point.  And probably, when you do hit this, there's some other "better" way to organize things, to avoid massive conditionals.  But in the current bytecode system I'm not seeing anything now.)

@@ +4259,5 @@
> +    bool letDecl = pn1 && pn1->isKind(PNK_LEXICALSCOPE);
> +    JS_ASSERT_IF(letDecl, pn1->isLet());
> +
> +    Rooted<StaticBlockObject*>
> +        blockObj(cx, letDecl ? &pn1->pn_objbox->object->as<StaticBlockObject>() : NULL);

nullptr now (if I haven't mentioned it anywhere else in here, do a s/NULL/nullptr/g overall).

@@ +4294,5 @@
> +    if (Emit1(cx, bce, JSOP_SWAP) < 0) // @@ITERATOR OBJ
> +        return false;
> +    if (Emit1(cx, bce, JSOP_NOTEARG) < 0)
> +        return false;
> +    if (Emit3(cx, bce, JSOP_CALL, ARGC_HI(0), ARGC_LO(0)) < 0) // ITER

Am I misremembering, or were you going to add an EmitCall method to abstract away the ARGC_HI/LO business?

@@ +4297,5 @@
> +        return false;
> +    if (Emit3(cx, bce, JSOP_CALL, ARGC_HI(0), ARGC_LO(0)) < 0) // ITER
> +        return false;
> +    CheckTypeSet(cx, bce, JSOP_CALL);
> +    if (Emit1(cx, bce, JSOP_UNDEFINED) < 0) // ITER RESULT

Add something like // Push a dummy result so that we properly enter iteration midstream.  or something.

@@ +4329,5 @@
> +    int loopDepth = bce->stackDepth;
> +#endif
> +
> +    // Emit code to assign result.value to the iteration variable.
> +    if (Emit1(cx, bce, JSOP_DUP) < 0)

// ITER RESULT RESULT

(..right?)

@@ +4331,5 @@
> +
> +    // Emit code to assign result.value to the iteration variable.
> +    if (Emit1(cx, bce, JSOP_DUP) < 0)
> +        return false;
> +    if (!EmitAtomOp(cx, cx->names().value, JSOP_GETPROP, bce))

// ITER RESULT VALUE

@@ +4333,5 @@
> +    if (Emit1(cx, bce, JSOP_DUP) < 0)
> +        return false;
> +    if (!EmitAtomOp(cx, cx->names().value, JSOP_GETPROP, bce))
> +        return false;
> +    if (!EmitAssignment(cx, bce, forHead->pn_kid2, JSOP_NOP, NULL))

Please update the comment by the !rhs bit in EmitAssignment to include for-of as well as for-in.

Also, // ITER RESULT BOGUS after this.  Right?  (Strangely JSOP_SETARG and all those seem to not change stack depth -- maybe a relic of ancient history when setter return values got propagated? -- so I have no idea what value exactly is there.  If I'm reading right.)

@@ +4335,5 @@
> +    if (!EmitAtomOp(cx, cx->names().value, JSOP_GETPROP, bce))
> +        return false;
> +    if (!EmitAssignment(cx, bce, forHead->pn_kid2, JSOP_NOP, NULL))
> +        return false;
> +    if (Emit1(cx, bce, JSOP_POP) < 0)

// ITER RESULT

@@ +4357,5 @@
> +    if (!EmitLoopEntry(cx, bce, NULL))
> +        return false;
> +
> +    if (Emit1(cx, bce, JSOP_POP) < 0)                          // ITER
> +        return false;

Am I reading correctly: this is (coming from the beginning of the loop) popping off the RESULT = undefined that was pushed just after the @@iterator call?

@@ +4380,5 @@
> +        return false;
> +    if (!EmitAtomOp(cx, cx->names().done, JSOP_GETPROP, bce))  // ITER RESULT DONE?
> +        return false;
> +
> +    ptrdiff_t beq = EmitJump(cx, bce, JSOP_IFEQ, top - bce->offset());

// ITER RESULT

@@ +4392,5 @@
> +        return false;
> +
> +    // Fixup breaks and continues.
> +    if (!PopStatementBCE(cx, bce))
> +        return false;

Please fix the comment in EmitNonLocalJumpFixup that refers only to for-let-in to also mention for-let-of as well.  Or maye you did and I just forgot about it, because there's an immediate-after assert that |stmt->down->type == STMT_FOR_IN_LOOP|, which is not the case here.  (Gag, what a mess.)

@@ +4402,5 @@
> +            return false;
> +    }
> +
> +    // Pop result, iter, and slots from the lexical block (if any).
> +    // FIXME: result.value is the completion value.

This shows up via eval only, right?  Add a test, mark it as failing, include the bug number for this in the in-test comment as to why it's marked as failing.

@@ +4727,5 @@
>  
>  static inline bool
>  EmitFor(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn, ptrdiff_t top)
>  {
> +    if (pn->pn_left->isKind(PNK_FORIN)) {

Hmm, we don't have a specific kind for for-of loops?  Please file a bug to add one, this is no good.

@@ +6863,5 @@
>  /*  4 */ {"for",            3},
>  
>  /*  5 */ {"while",          1},
>  /*  6 */ {"for-in",         1},
> +/*  6 */ {"for-of",         1},

7?

I guess we need to convert the source notes listing into a higher-order macro to get rid of this, at some point.  Mind filing that bug?

::: js/src/jit-test/tests/collections/Map-iterators-3.js
@@ +8,3 @@
>  m.clear();
>  m.set("a", 1);
> +assertIteratorResult(it.next(), undefined, true);  // iterator still closed

Hum.  Per bug 918075 comment 2, shouldn't this throw a TypeError?  Followup bug.

::: js/src/jit-test/tests/collections/Set-clear-iterators-3.js
@@ +8,3 @@
>  s.clear();
>  s.add("a");
> +assertIteratorResult(it.next(), undefined, true);

Same hum about whether this should throw TypeError.

::: js/src/jit-test/tests/collections/Set-iterator-add-2.js
@@ +8,2 @@
>  set.add("x");
> +assertIteratorResult(iter0.next(), undefined, true);  // already closed

SetIterator appears to store no internal state regarding its closed-ness, only an index into the related Set's element list.  So I think this should return "x", false here.  Please file a followup to fix this Set bug.

::: js/src/jit-test/tests/collections/iterator-1.js
@@ +9,5 @@
>      assertEq(iter.toString(), "[object " + obj.constructor.name + " Iterator]");
>  }
>  
> +// FIXME: Until arrays are converted to use the new iteration protocol,
> +// toString on this iterator doesn't work.

What bug's on file for this?  This is too important to get forgotten, so I'd like a bug number here.

::: js/src/jit-test/tests/for-of/next-arity.js
@@ +7,5 @@
> +function Iter() {
> +    function next() {
> +        log += 'n';
> +        if (arguments.length != 1)
> +            throw Error;

Might as well add arguments[0] !== undefined to this.

@@ +8,5 @@
> +    function next() {
> +        log += 'n';
> +        if (arguments.length != 1)
> +            throw Error;
> +        return { done: true }

get value() { throw 42; } might be nice here.

::: js/src/jsapi.cpp
@@ +4211,5 @@
>  
>      for (; fs->name; fs++) {
> +        RootedAtom atom(cx);
> +        if (fs->name[0] != '@' || fs->name[1] != '@')
> +            atom = Atomize(cx, fs->name, strlen(fs->name));

Lovely.  Like a Picasso.  Can I r+ this again?

::: js/src/jsiter.cpp
@@ +563,5 @@
>  
>  bool
>  js::GetIterator(JSContext *cx, HandleObject obj, unsigned flags, MutableHandleValue vp)
>  {
>      if (flags == JSITER_FOR_OF) {

Could be worth extracting the if-contents here into a GetForOfIterator method, and the rest of the method into a GetBlahIterator for whatever Blah is.  Do it in a separate patch from this one so as to not mix up code-moving blame with code-changing blame, rs=me on that.

@@ +571,3 @@
>              return false;
>  
> +        // Throw if obj[@@iterator]() isn't callable. js::Invoke is about to

There shouldn't be parens after obj[@@iterator].

@@ +750,5 @@
> +        return NULL;
> +
> +    RootedValue doneBool(cx, BooleanValue(done));
> +    if (!JSObject::defineProperty(cx, obj, cx->names().done, doneBool))
> +        return NULL;

These NULLs should be nullptr now.  Add #include "mozilla/NullPtr.h" if this file doesn't have it, too (probably does, this just changed in the last few days).

@@ +1350,5 @@
> +    InvokeArgs args(cx);
> +    if (!args.init(1))
> +        return false;
> +    args.setCallee(method);
> +    args.setThis(ObjectOrNullValue(iterator));

ObjectValue(*iterator)

@@ +1357,5 @@
> +        return false;
> +
> +    RootedObject resultObj(cx, ToObject(cx, args.rval()));
> +    if (!resultObj)
> +        return false;

Given that IMO the "if not an object, throw" behavior is the desirable behavior now, and it's what's currently specified, and Allen's already noted he thinks the current behavior is reasonable, I think we should do the work to make this implement that, here in this bug, now.

But *please* stick it in a followup patch, that can land separately/distinctly from all the changes in this mass'o'work.  Iteration on this patch (pun intended, natch) is slow for everybody.  A small patch fixing this, and the bytecode produced for for-of, and yield*, will review *much* faster than iterating on this patch.

@@ +1366,5 @@
> +    if (*done)
> +        vp.setUndefined();
> +    else if (!JSObject::getProperty(cx, resultObj, resultObj, cx->names().value, vp))
> +        return false;
> +    return true;

I'd slightly restructure to

if (*done) {
    vp.setUndefined();
    return true;
}
return JSObject::getProperty(cx, resultObj, resultObj, cx->names().value, vp);

if it were me.

::: js/src/jsiter.h
@@ +234,5 @@
> + * Create an object of the form { value: VALUE, done: DONE }.
> + * ES6 draft from 2013-09-05, section 25.4.3.4.
> + */
> +extern JSObject *
> +js_CreateItrResultObject(JSContext *cx, js::HandleValue value, bool done);

js::CreateItrResultObject
Attachment #810531 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 810531 [details] [diff] [review]
for-of calls @@iterator on iteratee v6

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

::: content/html/content/test/test_formelements.html
@@ +51,5 @@
>  is(names[8], "z", "Entry 9")
>  is(names[9], "something", "Entry 10")
>  is(names[10], "item", "Entry 11")
>  is(names[11], "namedItem", "Entry 12")
> +// Can't test the name of @@iterator.

Why not?

::: js/src/jsapi.cpp
@@ +4210,5 @@
>      RootedObject ctor(cx);
>  
>      for (; fs->name; fs++) {
> +        RootedAtom atom(cx);
> +        if (fs->name[0] != '@' || fs->name[1] != '@')

All names have length >= 2?
(Assignee)

Comment 46

6 years ago
Posted patch proposed fixup for jetpack (obsolete) — Splinter Review
Possible fixes for jetpack, trybuild here: https://tbpl.mozilla.org/?tree=Try&rev=35cc26f33b4b
(Assignee)

Comment 47

6 years ago
Hi,

(In reply to :Ms2ger (back and backlogged from being away) from comment #45)
> ::: content/html/content/test/test_formelements.html
> @@ +51,5 @@
> >  is(names[8], "z", "Entry 9")
> >  is(names[9], "something", "Entry 10")
> >  is(names[10], "item", "Entry 11")
> >  is(names[11], "namedItem", "Entry 12")
> > +// Can't test the name of @@iterator.
> 
> Why not?

Because it's a symbol.  OK it's not right now, it's a string, but the important thing about that string is its identity and not its spelling.  Its spelling could change to avoid the web platform fixing on the current spelling.

However... tc39 is still nailing this down, but it could be that when it changes to be a symbol, that it would not be returned by for-in.  I wanted to future-proof the test a bit, but it's more future-resistant than future-proof...

> ::: js/src/jsapi.cpp
> @@ +4210,5 @@
> >      RootedObject ctor(cx);
> >  
> >      for (; fs->name; fs++) {
> > +        RootedAtom atom(cx);
> > +        if (fs->name[0] != '@' || fs->name[1] != '@')
> 
> All names have length >= 2?

No.  Names of length 0 have fs->name[0] of NUL, which is not '@', so fs->name[1] isn't accessed.  Nasty, eh?
(In reply to Andy Wingo [:wingo] from comment #47)
> Hi,
> 
> (In reply to :Ms2ger (back and backlogged from being away) from comment #45)
> > ::: content/html/content/test/test_formelements.html
> > @@ +51,5 @@
> > >  is(names[8], "z", "Entry 9")
> > >  is(names[9], "something", "Entry 10")
> > >  is(names[10], "item", "Entry 11")
> > >  is(names[11], "namedItem", "Entry 12")
> > > +// Can't test the name of @@iterator.
> > 
> > Why not?
> 
> Because it's a symbol.  OK it's not right now, it's a string, but the
> important thing about that string is its identity and not its spelling.  Its
> spelling could change to avoid the web platform fixing on the current
> spelling.
> 
> However... tc39 is still nailing this down, but it could be that when it
> changes to be a symbol, that it would not be returned by for-in.  I wanted
> to future-proof the test a bit, but it's more future-resistant than
> future-proof...

Please keep these tests for now, then. We can deal with the future in the future.

> > ::: js/src/jsapi.cpp
> > @@ +4210,5 @@
> > >      RootedObject ctor(cx);
> > >  
> > >      for (; fs->name; fs++) {
> > > +        RootedAtom atom(cx);
> > > +        if (fs->name[0] != '@' || fs->name[1] != '@')
> > 
> > All names have length >= 2?
> 
> No.  Names of length 0 have fs->name[0] of NUL, which is not '@', so
> fs->name[1] isn't accessed.  Nasty, eh?

Quite :). Consider adding a comment?
(Assignee)

Comment 49

6 years ago
Comment on attachment 811035 [details] [diff] [review]
proposed fixup for jetpack

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

::: addon-sdk/source/lib/sdk/util/list.js
@@ +8,5 @@
>  };
>  
>  const { Class } = require('../core/heritage');
>  const listNS = require('../core/namespace').ns();
> +const { iteratorSymbol } = require('../util/iteration.js');

After fixing the bug here (shouldn't have the .js extension) I ran this: https://tbpl.mozilla.org/?tree=Try&rev=8baa9101d945

And it seems that there appears to be some issue XDR'ing star generators that causes the browser to core (!).  At least that's my theory.  I've been in meetings all today but if someone happens upon this bug and has an idea, I'd appreciate any thoughts.
(Assignee)

Updated

6 years ago
Depends on: 922028
(Assignee)

Comment 50

6 years ago
Hi,

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #44)
> @@ +1473,5 @@
> >      SetIteratorObject::finalize
> >  };
> >  
> >  const JSFunctionSpec SetIteratorObject::methods[] = {
> > +    JS_SELF_HOSTED_FN("@@iterator", "IteratorIdentity", 0, 0),
> 
> This just occurred to me at this point, but could you change all the
> "@@iterator" strings to js_std_iterator_str?  (Everything in
> CommonPropertyNames.h gets a js_##foo##_str const char[] to go with it.) 
> Then removal of the @@iterator name will cause every C++ use to turn into
> build bustage, when we do it.

So I had this before.  However at some point it's not going be simply the spelling of @@iterator that changes: the key will be a "well-known" symbol and not a string.  But how do you represent a symbol in a JSFunctionSpec?  So I thought, why not have the symbol be "@@iterator".  We make the DefineProperties function treat all keys that start with "@@" as well-known symbols, and if the @@-key isn't recognized, we abort.  At that point putting "@@iterator" as the key name in the JSFunctionSpec makes sense, because it's actually an indirection that will keep working when @@iterator becomes a symbol.

WDYT?

Andy
(Assignee)

Comment 51

6 years ago
Will push patch fixing nits, but a few more replies:

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #44)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +4333,5 @@
> > +    if (Emit1(cx, bce, JSOP_DUP) < 0)
> > +        return false;
> > +    if (!EmitAtomOp(cx, cx->names().value, JSOP_GETPROP, bce))
> > +        return false;
> > +    if (!EmitAssignment(cx, bce, forHead->pn_kid2, JSOP_NOP, NULL))
> 
> Also, // ITER RESULT BOGUS after this.  Right?  (Strangely JSOP_SETARG and
> all those seem to not change stack depth -- maybe a relic of ancient history
> when setter return values got propagated? -- so I have no idea what value
> exactly is there.  If I'm reading right.)

I think it's actually // ITER RESULT VALUE, as assignment is an expression, and it leaves its value on the stack for the continuation to use or drop as appropriate.

> @@ +4357,5 @@
> > +    if (!EmitLoopEntry(cx, bce, NULL))
> > +        return false;
> > +
> > +    if (Emit1(cx, bce, JSOP_POP) < 0)                          // ITER
> > +        return false;
> 
> Am I reading correctly: this is (coming from the beginning of the loop)
> popping off the RESULT = undefined that was pushed just after the @@iterator
> call?

This pop discards the previous iteration's result object.  For the first iteration, there's no previous result; we had pushed "undefined".  It might be possible to avoid this initial push-pop, but I think it's currently necessary given Ion's expectations as to what a loop looks like.

> @@ +4402,5 @@
> > +            return false;
> > +    }
> > +
> > +    // Pop result, iter, and slots from the lexical block (if any).
> > +    // FIXME: result.value is the completion value.
> 
> This shows up via eval only, right?  Add a test, mark it as failing, include
> the bug number for this in the in-test comment as to why it's marked as
> failing.

Thanks for the comment, it reminded me that I actually had this comment wrong.  The completion value is from the last trip through the loop.  I added a test and it happens to work already, though I don't know how.  So I removed this comment.

See http://people.mozilla.org/~jorendorff/es6-draft.html#sec-13.7.5.7 .

> @@ +4727,5 @@
> >  
> >  static inline bool
> >  EmitFor(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn, ptrdiff_t top)
> >  {
> > +    if (pn->pn_left->isKind(PNK_FORIN)) {
> 
> Hmm, we don't have a specific kind for for-of loops?  Please file a bug to
> add one, this is no good.

OK, bug 922066.

> @@ +6863,5 @@
> >  /*  4 */ {"for",            3},
> >  
> >  /*  5 */ {"while",          1},
> >  /*  6 */ {"for-in",         1},
> > +/*  6 */ {"for-of",         1},
> 
> 7?
> 
> I guess we need to convert the source notes listing into a higher-order
> macro to get rid of this, at some point.  Mind filing that bug?

Bug 922070.

> ::: js/src/jit-test/tests/collections/Map-iterators-3.js
> @@ +8,3 @@
> >  m.clear();
> >  m.set("a", 1);
> > +assertIteratorResult(it.next(), undefined, true);  // iterator still closed
> 
> Hum.  Per bug 918075 comment 2, shouldn't this throw a TypeError?  Followup
> bug.

Nope; map iterators are specified by http://people.mozilla.org/~jorendorff/es6-draft.html#sec-23.1.5.2.2, which can be iterated after they are "closed".  And actually, I'm not sure about that spec language -- it's written using a loop, but it's specified to not be an generator but a prototype method...

::: js/src/jit-test/tests/collections/Set-iterator-add-2.js
> @@ +8,2 @@
> >  set.add("x");
> > +assertIteratorResult(iter0.next(), undefined, true);  // already closed
> 
> SetIterator appears to store no internal state regarding its closed-ness,
> only an index into the related Set's element list.  So I think this should
> return "x", false here.  Please file a followup to fix this Set bug.

Hummmmmmmmmmmmm.  I think this is a bug in the ES6 spec.  https://bugs.ecmascript.org/show_bug.cgi?id=2003.

> ::: js/src/jit-test/tests/collections/iterator-1.js
> @@ +9,5 @@
> >      assertEq(iter.toString(), "[object " + obj.constructor.name + " Iterator]");
> >  }
> >  
> > +// FIXME: Until arrays are converted to use the new iteration protocol,
> > +// toString on this iterator doesn't work.
> 
> What bug's on file for this?  This is too important to get forgotten, so I'd
> like a bug number here.

Bug 919948.

> @@ +1357,5 @@
> > +        return false;
> > +
> > +    RootedObject resultObj(cx, ToObject(cx, args.rval()));
> > +    if (!resultObj)
> > +        return false;
> 
> Given that IMO the "if not an object, throw" behavior is the desirable
> behavior now, and it's what's currently specified, and Allen's already noted
> he thinks the current behavior is reasonable, I think we should do the work
> to make this implement that, here in this bug, now.
> 
> But *please* stick it in a followup patch, that can land
> separately/distinctly from all the changes in this mass'o'work.  Iteration
> on this patch (pun intended, natch) is slow for everybody.  A small patch
> fixing this, and the bytecode produced for for-of, and yield*, will review
> *much* faster than iterating on this patch.

Roger.

(At this point I think I've addressed all of the feedback from comment 44; new patch soon)
(Assignee)

Comment 52

6 years ago
OK, I think this patch addresses all comments.  Will push a trybuild with it and a fixed jetpack patch.
Attachment #810531 - Attachment is obsolete: true
Attachment #812023 - Flags: review+
(Assignee)

Comment 53

6 years ago
r? to gozala for the jetpack part of things
Attachment #811035 - Attachment is obsolete: true
Attachment #812029 - Flags: review?(rFobic)
(Assignee)

Comment 54

6 years ago
attachment 812023 [details] [diff] [review] is r=Waldo, fwiw
(In reply to Andy Wingo [:wingo] from comment #50)
> So I thought, why not have the symbol be "@@iterator".  We make the
> DefineProperties function treat all keys that start with "@@" as well-known
> symbols, and if the @@-key isn't recognized, we abort.

That is a good idea.
Comment on attachment 812029 [details] [diff] [review]
Adapt addon SDK custom iterators to work with new for-of behavior

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

Thanks Andy, looks good!
Please do not land this change to mc though (we have one way sync from git),
we'll land it to our canonical git repo.
Attachment #812029 - Flags: review?(rFobic) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #55)
> (In reply to Andy Wingo [:wingo] from comment #50)
> > So I thought, why not have the symbol be "@@iterator".  We make the
> > DefineProperties function treat all keys that start with "@@" as well-known
> > symbols, and if the @@-key isn't recognized, we abort.
> 
> That is a good idea.

That seems reasonable-ish in some short run.  Longer run, I'm not sure I like string-overloading on general principle (every caller has to check its input string, and if that's dynamically computed it'd be easy to forget), but probably that should be fixed with an entirely different interface, not by building further atop JSPropertySpec.
(Assignee)

Comment 58

6 years ago
Carrying r+ from Waldo; will comment on the one change.
Attachment #812023 - Attachment is obsolete: true
Attachment #812619 - Flags: review+
(Assignee)

Comment 59

6 years ago
Comment on attachment 812619 [details] [diff] [review]
for-of calls @@iterator on iteratee v8

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

::: js/src/builtin/Iterator.js
@@ +20,5 @@
> +    try {
> +        return IteratorResult(iter.next(arg), false);
> +    } catch (e) {
> +        if (e instanceof std_StopIteration)
> +            return IteratorResult(undefined, true);

So, this used to return IteratorResult(e.value, true), as suggested by an early ES6 generators draft.  However this was making xpcshell tests fail, because the devtools tests would balk on seeing the "e.value is not defined" strict-mode warning.  Because nothing depended on propagating the result from the StopIteration, I reverted to the safe situation of just resulting to "undefined".  We can do something else in the future but hopefully this part of the present will stay in the past, is the thing.

With this I've finally gotten all green tests and am now running a fresh huge trybuild: https://tbpl.mozilla.org/?tree=Try&rev=f182b77b1b0c  If that goes green I'll set checkin-needed.  Finally.  Thank you to all for your patience and help with this patch.  I am never making a patch this big again ;)
(Assignee)

Comment 60

6 years ago
Seems the try-build is looking good.  Let's do this!

As a reminder to the friendly checkin folk: the patch I marked as r+ is r=Waldo, and please don't check in the one marked r+ by rFobic -- the jetpack changes flow into m-c from their git repo.  Please leave the bug open.
Keywords: checkin-needed
Landed the Jetpack patch to fix perma-orange on inbound. The Jetpack team can resolve the conflict the next time they merge from upstream.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d72b823b67f
Whiteboard: [leave open]
(Assignee)

Comment 64

6 years ago
In summary the spread-call-maxargs.js test timed out on the ASAN debug build.  This test does:

  var a = []
  a.length = getMaxArgs() + 1;
  try { f(...a) } catch (e) { ... }

and the same for eval and constructor calls.

So, this test was slow (around 4 seconds on my machine) and I made it slower (see bug 923028), though I'll speed it up in bug 919948.  I can imagine that a concurrently running --ion-eager test could force the machine into thrashing, which with ASAN and all could conceivably cause the test to time out.  It's a stretch -- the timeout is 150 seconds -- but possible.

I propose to add // |jit-test| slow; to this test.  WDYT, Jason or Jeff?
(Assignee)

Updated

6 years ago
Blocks: 923160
(Assignee)

Comment 65

6 years ago
Attached patch is the same, but skips the spread-call-maxargs test on debug && asan builds.
Attachment #812619 - Attachment is obsolete: true
Attachment #813149 - Flags: review+
(Assignee)

Comment 66

6 years ago
rs=jorendorff for the test workaround; will investigate later to see if bug 919948 fixes it.  Please check in both patches.  Cheers.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a4f96de49668
https://hg.mozilla.org/mozilla-central/rev/1065b60241f7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Comment 69

6 years ago
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/1910af733de7bd7ecec815aa2cf86947c21e1925
Bug 907077: Adapt addon SDK custom iterators to work with new for-of behavior r=rFobic
So it appears that this has introduced a performance regression in mozContacts.  See bug 927079.

Is it expected that conforming to the new standard would slow things down?
Flags: needinfo?(wingo)
I don't believe it should, ultimately.  But along the way there might be bumps, as with any bit of work.

Looking at dom/contacts/ContactManager.js there's only one for-of, on arrays.  Maybe bug 919948 is at fault?  Could be worth testing with patches there, if they apply cleanly.  If that's not it, my ability to eyeball this isn't enough to figure out the problem.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #71)
> I don't believe it should, ultimately.  But along the way there might be
> bumps, as with any bit of work.

This is new to me, so I may be confused, but it looks like es6 for-of now requires iterators that return { value: foo, done: false }.  Is that correct?  Is there any concern about baking an object creation into every iteration of a loop?  The profiling in bug 927079 seems to point towards the memory allocations as cause of the slow down.

> Looking at dom/contacts/ContactManager.js there's only one for-of, on
> arrays.  Maybe bug 919948 is at fault?  Could be worth testing with patches
> there, if they apply cleanly.  If that's not it, my ability to eyeball this
> isn't enough to figure out the problem.

Trying this now.  The patches don't apply cleanly, but I'm trying to manually merge.
> This is new to me, so I may be confused, but it looks like es6 for-of now
> requires iterators that return { value: foo, done: false }.  Is that
> correct?

Yes.

> Is there any concern about baking an object creation into every
> iteration of a loop?

There is some concern about it. Brendan is dead set against it. But at the moment this is what's in the ES6 draft spec. If we end up showing that the objects are too expensive, we can take that back to TC39.

> The profiling in bug 927079 seems to point towards the
> memory allocations as cause of the slow down.

I'm not totally convinced. Right now for-of loops over arrays run a bunch of code in js/src/builtin/Iterator.js that we really don't want. There's even a WeakMap involved... and a try-catch block... it's not pretty.

The patch in bug 919948 will fix it; let's get that landed and see where we stand.
Unfortunately I just tested with bug 919948 applied and got similar results.

Here is a profile:

  http://people.mozilla.org/~bgirard/cleopatra/#report=ca341774af50ac0669e4d47d869f7344d8243555

(Again, that huge spike is an artifact in the native stack unwinding on b2g at the moment and can be ignored.)

Its a bit hard to see because there are so many paths to ContactManager's validateArrayField(), but they all seem to show time in the IteratorResult() constructor function.

Also, it appears that we may be paying a double penalty due to the std_Object_create(null) and subsequent property assignments.  Those assignments appear to be trigger a secondary allocation to store the property.  This patch to replace use inline object syntax cuts the time penalty by 100ms or about 33%:

 function IteratorResult(value, done) {
-    var result = std_Object_create(null);
-    result.value = value;
-    result.done = done;
-    return result;
+    return { value: value, done: done };
 }

I don't know if this is a valid patch, though, since I think the prototype will be different.
(Assignee)

Comment 75

6 years ago
(In reply to Ben Kelly [:bkelly] from comment #70)
> So it appears that this has introduced a performance regression in
> mozContacts.  See bug 927079.

> Is it expected that conforming to the new standard would slow things down?

As Jeff said, ultimately no, but there could be bumps along the way.

(In reply to Jason Orendorff [:jorendorff] from comment #73)

> > Is there any concern about baking an object creation into every
> > iteration of a loop?
> 
> There is some concern about it. Brendan is dead set against it.

Are you certain about this last bit?  I have only seen agreement from Brendan on the list.

Anyway the way out here is scalar replacement, and then it will be as fast as it can be.  Dart does this for its iteration protocol, which is much the same.
Flags: needinfo?(wingo)
(In reply to Andy Wingo [:wingo] from comment #75)
> (In reply to Jason Orendorff [:jorendorff] from comment #73)
> 
> > > Is there any concern about baking an object creation into every
> > > iteration of a loop?
> > 
> > There is some concern about it. Brendan is dead set against it.
> 
> Are you certain about this last bit?  I have only seen agreement from
> Brendan on the list.
> 
> Anyway the way out here is scalar replacement, and then it will be as fast
> as it can be.  Dart does this for its iteration protocol, which is much the
> same.

Sayeth Brendan on es-discuss:
"I quite agree [with concerns about object allocations], and proposed StopIteration (in general, a class, per PEP380) for years. But this heap allocation is relatively straightforward to avoid in modern JITs, compared to other optimizations already done for JS. When for-of drivers the iteration, there's no need for the allocation, which is typically done in a return expression[.]"
(Assignee)

Comment 78

6 years ago
(In reply to Ben Kelly [:bkelly] from comment #74)
> Unfortunately I just tested with bug 919948 applied and got similar results.
> 
> Here is a profile:
> 
>  
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=ca341774af50ac0669e4d47d869f7344d8243555
> 
> (Again, that huge spike is an artifact in the native stack unwinding on b2g
> at the moment and can be ignored.)
> 
> Its a bit hard to see because there are so many paths to ContactManager's
> validateArrayField(), but they all seem to show time in the IteratorResult()
> constructor function.
> 
> Also, it appears that we may be paying a double penalty due to the
> std_Object_create(null) and subsequent property assignments.  Those
> assignments appear to be trigger a secondary allocation to store the
> property.  This patch to replace use inline object syntax cuts the time
> penalty by 100ms or about 33%:
> 
>  function IteratorResult(value, done) {
> -    var result = std_Object_create(null);
> -    result.value = value;
> -    result.done = done;
> -    return result;
> +    return { value: value, done: done };
>  }
> 
> I don't know if this is a valid patch, though, since I think the prototype
> will be different.

You know, I think this is probably a correct change, given that the draft now specifies that the result object has %ObjectPrototype% as its prototype.
Great, let's do that. In fact, let's remove IteratorResult entirely and just use an object literal at each call site. Separate bug, please. I can promise an instant review. :)
(In reply to Jason Orendorff [:jorendorff] from comment #80)
> Great, let's do that. In fact, let's remove IteratorResult entirely and just
> use an object literal at each call site. Separate bug, please. I can promise
> an instant review. :)

I wrote bug 927649 to track using object literal syntax instead IteratorResult().
Whiteboard: [DocArea=JS]
Should this have been on the Firefox 27 for developers page?
(Reporter)

Comment 83

5 years ago
(In reply to Mike Kaply (:mkaply) from comment #82)
> Should this have been on the Firefox 27 for developers page?
Hmm... Yes... I actually suffered from that change in an addon of mine (but 27 was in Aurora then). I'll look at it tomorrow if no one did it.

@Andy I feel the bug changed of purpose enough so that its current summary doesn't reflect what actually happened. Could you modify it so it better reflects the changes? Thanks.
Flags: needinfo?(wingo)
(Assignee)

Comment 84

5 years ago
I modified the title.  Sorry for stealing your bug here, David.  The new state of affairs is documented in the blog post linked from the FF27 release notes: http://wingolog.org/archives/2013/10/07/es6-generators-and-iteration-in-spidermonkey.

Besides what is written there, the change is that for-of no longer uses the iterator-and-StopIteration protocol; it uses the @@iterator and boxed {value, done} object protocol.
Flags: needinfo?(wingo)
Summary: Hide the "iterator"-based for-of customization from web content → Remove "iterator"-based iterator customization in favor of ES6 @@iterator
(Assignee)

Updated

5 years ago
Summary: Remove "iterator"-based iterator customization in favor of ES6 @@iterator → Replace "iterator"-based for-of customization with ES6 @@iterator
(Reporter)

Comment 85

5 years ago
(In reply to Andy Wingo [:wingo] from comment #84)
> I modified the title.
Thanks

> Sorry for stealing your bug here, David.
No problem :-) Things move forward, that's what is most important

> The new
> state of affairs is documented in the blog post linked from the FF27 release
> notes:
> http://wingolog.org/archives/2013/10/07/es6-generators-and-iteration-in-
> spidermonkey.
> 
> Besides what is written there, the change is that for-of no longer uses the
> iterator-and-StopIteration protocol; it uses the @@iterator and boxed
> {value, done} object protocol.
Ok, re-modifying the title accordingly. I'm removing the mention of ES6 as ES6 mandates the use of symbols. The '@@iterator' string is just a temporary measure until then.
It's a bit long now, but clearly states what happened (feel free to re-fix if it's still ambiguous).

Just be sure on things:
Did the legacy SpiderMonkey iterator protocol got removed? If so, in which bug (is it this one)?

I'm marking this bug as site-compat and addon-compat as what is exposed to both has been changed in this bug. In practice, the site-compat shouldn't really affect website since for-of loops aren't really used that much yet. Feel free to re-change if I misused these keywords.
Summary: Replace "iterator"-based for-of customization with ES6 @@iterator → for-of loops: Replace "iterator"-based customization with "@@iterator" (until ES6 symbols are implemented) and use ES6 iterator protocol (moving away from legacy SpiderMonkey iterator protocol)
(Reporter)

Comment 86

5 years ago
(In reply to David Bruant from comment #83)
> (In reply to Mike Kaply (:mkaply) from comment #82)
> > Should this have been on the Firefox 27 for developers page?
> Hmm... Yes... I actually suffered from that change in an addon of mine
For anyone interested, the symptoms were that my program would get stuck in an infinite loop, because the iterating code would expect a StopIteration to be thrown at some point, but the new iterator protocol doesn't throw anything any longer.
I fixed the iterator protocol related bugs in this specific commit:
https://github.com/DavidBruant/OoI/commit/b00be9c8a436494cad2eafb62774d1e2228fdc92
You need to log in before you can comment on or make changes to this bug.