919948 - Convert Array.prototype.@@iterator to use new iteration protocol

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andy Wingo [:wingo]]

Attachment: array-iterator

The attached patch builds on bug 907077 to implement array iteration directly in JavaScript, using the { value, done } protocol, instead of as a shim around the StopIteration protocol implemented in C++.

It removes JS_ArrayIterator from the public API, and removes "iterator" from array, string, and other objects.

Note the incompatible change in the for-of/next-3.js test.

Comment 1

[André Bargull [:anba]]

Comment on attachment 809083 [details] [diff] [review]
array-iterator

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

::: js/src/builtin/Array.js
@@ +495,5 @@
> +    var itemKind = priv.iterationKind;
> +    // FIXME: This should be ToLength, which clamps at 2**53.
> +    var len = TO_UINT32(a.length);
> +    // FIXME: Support sparse iteration.
> +    assert(!/sparse/.test(itemKind), itemKind);

May not work if RegExp.prototype.test was deleted.

@@ +498,5 @@
> +    // FIXME: Support sparse iteration.
> +    assert(!/sparse/.test(itemKind), itemKind);
> +
> +    if (index >= len) {
> +        priv.nextIndex = 1/0;

TO_UINT32(Infinity) === 0! Instead set priv.nextIndex to 0xffffffff.

@@ +528,5 @@
> +    props.next.configurable = true;
> +    props.next.writable = true;
> +
> +    props[std_iterator] = std_Object_create(null);
> +    props[std_iterator].value = IteratorIdentity;

I don't think it's possible to use a shared IteratorIdentity function here. That means ArrayIterator needs its own ArrayIteratorIdentity function.

Comment 2

[Andy Wingo [:wingo]]

Thank you for the comments, André!  Will fix.

Comment 3

[Jason Orendorff [:jorendorff]]

Andy, given bug 907077 comment 36, should I review this now or wait?

Preliminary comment: sparse iteration is never going to be a thing. Please remove all traces!

I'd rather have .iterationKind be an int. The spec uses a string, but the information being conveyed there is really just an enum. Strings aren't terrible, but using an int will surely eliminate a branch somewhere, if not more.

Comment 4

[Jason Orendorff [:jorendorff]]

(Yes, sparse iteration is in the spec, but it is not observable and no one on TC39 is interested in exposing it. Users have no desire for it. There is already a "comment" on the sparse iteration pseudocode, in the Word doc, to the effect that it may go away. I'm quite sure it will.)

Comment 5

[Andy Wingo [:wingo]]

Attachment: Convert Array.prototype.@@iterator to use new iteration protocol v2

Comment 6

[Andy Wingo [:wingo]]

Comment on attachment 809857 [details] [diff] [review]
Convert Array.prototype.@@iterator to use new iteration protocol v2

New patch defines the ArrayIterator class in C++, but next in JS.  That way we can store private state in reserved slots.  WDYT?

Comment 7

[Andy Wingo [:wingo]]

Attachment: Convert Array.prototype.@@iterator to use new iteration protocol v3

Comment 8

[Andy Wingo [:wingo]]

Comment on attachment 813054 [details] [diff] [review]
Convert Array.prototype.@@iterator to use new iteration protocol v3

Updated patch adapts to changes in m-c about JS_SELF_HOSTED_FN and various other things.  It also adds back the array-iterators-gc test, which works again with the new reserved-slot implementation.

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 813054 [details] [diff] [review]
Convert Array.prototype.@@iterator to use new iteration protocol v3

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

r=me, just a few nits.

::: js/src/builtin/Array.js
@@ +489,5 @@
> +    return this;
> +}
> +
> +function ArrayIteratorNext() {
> +    if (!IsObject(this) || !HaveSameClass(this, GetArrayIteratorPrototype()))

Followup bug: the ArrayIteratorPrototype should not pass this test, since (in the spec language) it does not have the internal properties of an Array Iterator Instance; <<ArrayIteratorPrototype>>.next() should throw a TypeError.

(However Object.prototype.toString.call(<<ArrayIteratorPrototype>>) does need to return "[object Array Iterator]".)

@@ +497,5 @@
> +    var index = UnsafeGetReservedSlot(this, ARRAY_ITERATOR_SLOT_NEXT_INDEX);
> +    var itemKind = UnsafeGetReservedSlot(this, ARRAY_ITERATOR_SLOT_ITEM_KIND);
> +
> +    // FIXME: This should be ToLength, which clamps at 2**53.
> +    if (index >= TO_UINT32(a.length)) {

Could you please file the followup bug to switch over to ToLength in all the array algorithms for ES6?

::: js/src/builtin/Utilities.js
@@ +60,5 @@
>  var std_Object_create = Object.create;
>  var std_Object_defineProperty = Object.defineProperty;
>  var std_Object_getOwnPropertyNames = Object.getOwnPropertyNames;
>  var std_Object_hasOwnProperty = Object.prototype.hasOwnProperty;
> +var std_Object_prototype = Object.prototype;

Where is this used?

::: js/src/jit-test/tests/for-of/array-iterator-shrinking.js
@@ +1,1 @@
>  // If an array is truncated to the left of an iterator it, it.next() throws StopIteration.

Update the comment, please.

::: js/src/jit-test/tests/for-of/array-iterator-surfaces-1.js
@@ +1,1 @@
>  // Superficial tests of the Array.prototype.iterator builtin function and its workalikes.

Update the comment ("Array.prototype[@@iterator]").

::: js/src/jit-test/tests/for-of/next-3.js
@@ +1,3 @@
> +// Iterators from another compartment work with their own .next method
> +// when called from another compartment, but not with the other
> +// compartment's .next method.

Lame.

My reading of ES6 is that this is required to work cross-Realm, so when you get a chance, let's talk about how it would be possible to fix it. In general it should be possible for self-hosted functions to be nongeneric without headaches. I think sfink may already have filed a bug in this general area.

::: js/src/jit-test/tests/for-of/string-iterator-generic.js
@@ +5,3 @@
>  
>  function test(obj) {
> +    var it = Array.prototype[std_iterator].call(obj);

Preexisting: I just realized this test does not actually test what it's supposed to test. Please s/Array/String/g on this line if you agree.

Comment 10

[André Bargull [:anba]]

(In reply to Jason Orendorff [:jorendorff] from comment #9)
> ::: js/src/jit-test/tests/for-of/string-iterator-generic.js
> @@ +5,3 @@
> >  
> >  function test(obj) {
> > +    var it = Array.prototype[std_iterator].call(obj);
> 
> Preexisting: I just realized this test does not actually test what it's
> supposed to test. Please s/Array/String/g on this line if you agree.

And then close bug 866604. :-)

Comment 11

[Andy Wingo [:wingo]]

Patch latency versus API churn, part N: they removed NewObjectWithClassPrototype in bug 922283.  Grrrrrrrrrrrrrr.

Comment 12

[Andy Wingo [:wingo]]

Thanks for the review.  Addressed nits.

Unfortunately in the meantime, NewObjectFromClassPrototype went away.  So instead of exposing GetArrayIteratorPrototype, I'm exposing NewArrayIterator and HasArrayIteratorClass.  I figured specific was better than generic until we can figure out the right patterns.  Perhaps this is worth a new round; dunno.  Will decide when I see the interdiff.

(In reply to Jason Orendorff [:jorendorff] from comment #9)
> ::: js/src/builtin/Array.js
> @@ +489,5 @@
> > +    return this;
> > +}
> > +
> > +function ArrayIteratorNext() {
> > +    if (!IsObject(this) || !HaveSameClass(this, GetArrayIteratorPrototype()))
> 
> Followup bug: the ArrayIteratorPrototype should not pass this test, since
> (in the spec language) it does not have the internal properties of an Array
> Iterator Instance; <<ArrayIteratorPrototype>>.next() should throw a
> TypeError.

Bug 924059.

> Could you please file the followup bug to switch over to ToLength in all the
> array algorithms for ES6?

Bug 924058.

> ::: js/src/builtin/Utilities.js
> @@ +60,5 @@
> >  var std_Object_create = Object.create;
> >  var std_Object_defineProperty = Object.defineProperty;
> >  var std_Object_getOwnPropertyNames = Object.getOwnPropertyNames;
> >  var std_Object_hasOwnProperty = Object.prototype.hasOwnProperty;
> > +var std_Object_prototype = Object.prototype;
> 
> Where is this used?

Good catch, removed.

> ::: js/src/jit-test/tests/for-of/next-3.js
> @@ +1,3 @@
> > +// Iterators from another compartment work with their own .next method
> > +// when called from another compartment, but not with the other
> > +// compartment's .next method.
> 
> Lame.
> 
> My reading of ES6 is that this is required to work cross-Realm, so when you
> get a chance, let's talk about how it would be possible to fix it. In
> general it should be possible for self-hosted functions to be nongeneric
> without headaches. I think sfink may already have filed a bug in this
> general area.

I don't know what the deal is.  I thought the reworked HasArrayIteratorClass would fix this but it didn't.  Bug 924059.

> ::: js/src/jit-test/tests/for-of/string-iterator-generic.js
> @@ +5,3 @@
> >  
> >  function test(obj) {
> > +    var it = Array.prototype[std_iterator].call(obj);
> 
> Preexisting: I just realized this test does not actually test what it's
> supposed to test. Please s/Array/String/g on this line if you agree.

Good catch, done.

Comment 13

[Andy Wingo [:wingo]]

Attachment: Convert Array.prototype.@@iterator to use new iteration protocol v4

Comment 14

[Andy Wingo [:wingo]]

Comment on attachment 814053 [details] [diff] [review]
Convert Array.prototype.@@iterator to use new iteration protocol v4

Quick r? for the NewArrayIterator / HasArrayIteratorClass intrinsics.  TIA.

Comment 15

[Andy Wingo [:wingo]]

Comment on attachment 814053 [details] [diff] [review]
Convert Array.prototype.@@iterator to use new iteration protocol v4

r? to bz for Codegen.py change, as suggested by Ms2ger

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 814053 [details] [diff] [review]
Convert Array.prototype.@@iterator to use new iteration protocol v4

r=me on the Codegen.py bit.

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 814053 [details] [diff] [review]
Convert Array.prototype.@@iterator to use new iteration protocol v4

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

r=me.

::: js/src/jit-test/tests/for-of/array-iterator-shrinking.js
@@ +10,2 @@
>  it.next();
>  it.next();

Delete lines 10 and 11 to retain the original meaning of the test, please.

::: js/src/vm/SelfHosting.cpp
@@ +493,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    JS_ASSERT(args.length() == 1);
> +    JS_ASSERT(args[0].isObject());
> +
> +    args.rval().setBoolean(args[0].toObject().getClass() == &ArrayIteratorObject::class_);

.toObject().is<ArrayIteratorObject>()

@@ +578,5 @@
>  
>      JS_FN("GetIteratorPrototype",    intrinsic_GetIteratorPrototype,    0,0),
>  
> +    JS_FN("NewArrayIterator",        intrinsic_NewArrayIterator,        0,0),
> +    JS_FN("HasArrayIteratorClass",   intrinsic_HasArrayIteratorClass,   1,0),

If you wanted to call this IsArrayIterator, I'd be ok with that.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: un-bitrotted patch

In case it helps, here is an un-bitrotted patch I used to test the issue in bug 907077 today.  It compiles and I verified it passes the tests.

Note, it looks like we are removing IteratorResult() in bug 927649.  Once that lands you will need to replace calls to that function in this patch with object literals.

Comment 19

[Andy Wingo [:wingo]]

Attachment: Convert Array.prototype.@@iterator to use new iteration protocol r=jorendorff

Comment 20

[Andy Wingo [:wingo]]

Comment on attachment 818152 [details]
un-bitrotted patch

Thanks for this, bkelly, but I had something already that addressed jorendorff's comments, and now has adapted to bug 927649 landing.  Cheers.

Comment 21

[Till Schneidereit [:till]]

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

Comment 22

[Carsten Book [:Tomcat]]

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