Bug 141351

Summary: Upgrade ES6 Iterator interfaces
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, darin, dino, fpizlo, ggaren, joepeck, mark.lam, ossy, ysuzuki
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 142575    
Bug Blocks: 142080    
Attachments:
Description Flags
prototype
none
prototype
none
prototype
none
prototype
none
Patch
none
Patch
none
Patch
none
Patch fpizlo: review+

Description Yusuke Suzuki 2015-02-06 22:25:33 PST
Upgrade ES6 Iterator interfaces
Comment 1 Yusuke Suzuki 2015-02-06 22:27:45 PST
In the JSC's iterator interface, using [[iteratorNext]] for next() method. In the latest spec, it is exposed as Iterator.prototype.next()
Comment 2 Yusuke Suzuki 2015-02-11 11:46:41 PST
https://bugs.webkit.org/show_bug.cgi?id=122532
Need to consider about this patch.
Now, iterator interface in ES6 is reverted into { value / done } style.
And `next` function can be easily observed by users.
Comment 3 Yusuke Suzuki 2015-02-11 11:48:10 PST
And this
https://bugs.webkit.org/show_bug.cgi?id=122575
Comment 4 Yusuke Suzuki 2015-02-12 12:44:24 PST
Created attachment 246467 [details]
prototype

rev1 prototype
Comment 5 Yusuke Suzuki 2015-02-12 16:49:40 PST
Created attachment 246489 [details]
prototype

rev2 prototype
Comment 6 Yusuke Suzuki 2015-02-12 20:20:32 PST
Created attachment 246500 [details]
prototype

rev3 prototype
Comment 7 Yusuke Suzuki 2015-02-16 17:59:08 PST
Spawned from https://esdiscuss.org/topic/performance-of-iterator-next-as-specified#content-15

Thank you for your reply.
So leveraging this escape analysis, possiblly, implementing ArrayIteratorPrototype.next in JavaScript in builtin/ looks better than using a intrinsic.
Comment 8 Filip Pizlo 2015-02-16 18:14:01 PST
(In reply to comment #7)
> Spawned from
> https://esdiscuss.org/topic/performance-of-iterator-next-as-
> specified#content-15
> 
> Thank you for your reply.
> So leveraging this escape analysis, possiblly, implementing
> ArrayIteratorPrototype.next in JavaScript in builtin/ looks better than
> using a intrinsic.

Yes.
Comment 9 Yusuke Suzuki 2015-02-16 23:12:39 PST
Created attachment 246730 [details]
prototype

rev4 prototype
Comment 10 Yusuke Suzuki 2015-02-16 23:50:23 PST
After https://bugs.webkit.org/show_bug.cgi?id=141640 is fixed, we can leverage op_is_object to check whether iterator results are object or not.
Comment 11 Joseph Pecoraro 2015-02-20 20:07:11 PST
Comment on attachment 246730 [details]
prototype

Awesome! I'm excited about this.

It looks like you fixed up Promise.all(iterable). I look forward to "new Set(iterable)", because unfortunately WebKit handles Set construction differently from others right now:

  js> s = new Set([1,2,3])
  < Set

  js> s.size
  < 1 // expected 3.

That said, it would be a behavior change we'd need to be careful about.
Comment 12 Dean Jackson 2015-02-22 02:11:51 PST
(In reply to comment #11)
> Comment on attachment 246730 [details]
> prototype
> 
> Awesome! I'm excited about this.
> 
> It looks like you fixed up Promise.all(iterable). I look forward to "new
> Set(iterable)", because unfortunately WebKit handles Set construction
> differently from others right now:
> 
>   js> s = new Set([1,2,3])
>   < Set
> 
>   js> s.size
>   < 1 // expected 3.
> 
> That said, it would be a behavior change we'd need to be careful about.

Do you think so? I would think that not many people are using Set yet and, if they are, they probably are simply doing new Set(), and adding elements afterwards.

I think it's better to make the breaking change sooner rather than later.
Comment 13 Yusuke Suzuki 2015-02-22 09:44:48 PST
(In reply to comment #12)
> (In reply to comment #11)
> > Comment on attachment 246730 [details]
> > prototype
> > 
> > Awesome! I'm excited about this.
> > 
> > It looks like you fixed up Promise.all(iterable). I look forward to "new
> > Set(iterable)", because unfortunately WebKit handles Set construction
> > differently from others right now:
> > 
> >   js> s = new Set([1,2,3])
> >   < Set
> > 
> >   js> s.size
> >   < 1 // expected 3.
> > 
> > That said, it would be a behavior change we'd need to be careful about.
> 
> Do you think so? I would think that not many people are using Set yet and,
> if they are, they probably are simply doing new Set(), and adding elements
> afterwards.
> 
> I think it's better to make the breaking change sooner rather than later.

Personally I think new Set(iterable) change doesn't break the things in *web* applications
because this upgraded interface is already shipped by V8 in Chrome. So web applications code
may consider the both cases when using Set.
However, when changing it, it is necessary to check the use of this in the inspector code :)
Comment 14 Joseph Pecoraro 2015-02-22 23:53:15 PST
> Personally I think new Set(iterable) change doesn't break the things in *web* applications
> because this upgraded interface is already shipped by V8 in Chrome. So web applications code
> may consider the both cases when using Set.

What I mean is: Safari 8 has already shipped an implementation of Set. In that, "new Set(array)" creates a Set with 1 item in it. This change, would iterate the array and put each array item into the Set. That is an incompatible change.


> However, when changing it, it is necessary to check the use of this in the inspector code :)

Heh, we do try to make use of every little feature ;).

However, we do not make use of "new Set(...)" at all. I just checked. But I have been aware of this discrepancy and have been avoiding using any special "new Set" syntax for just this reason (in case JSC makes this change which would break us).

After thinking about it, I'm still a bit disappointed in the API. To create a union of 3 arrays, the "simple" approach is ugly:

  var union = new Set(array1.concat(array2).concat(array3))

It would be cool if the spec allowed variables arguments so I could do:

  var union = new Set(array1, array2, array3);

But, 1 gets the job done I suppose.
Comment 15 Yusuke Suzuki 2015-02-24 19:45:14 PST
(In reply to comment #14)
> What I mean is: Safari 8 has already shipped an implementation of Set. In
> that, "new Set(array)" creates a Set with 1 item in it. This change, would
> iterate the array and put each array item into the Set. That is an
> incompatible change.
>
> Heh, we do try to make use of every little feature ;).
> 
> However, we do not make use of "new Set(...)" at all. I just checked. But I
> have been aware of this discrepancy and have been avoiding using any special
> "new Set" syntax for just this reason (in case JSC makes this change which
> would break us).

Ah, yes. After aligning JSC to the latest spec, we can use the constructor.

> After thinking about it, I'm still a bit disappointed in the API. To create
> a union of 3 arrays, the "simple" approach is ugly:
> 
>   var union = new Set(array1.concat(array2).concat(array3))
> 
> It would be cool if the spec allowed variables arguments so I could do:
> 
>   var union = new Set(array1, array2, array3);
> 
> But, 1 gets the job done I suppose.

new Set(a1, a2, a3) API looks nice.
Investigating python's API, python's set also takes set(iterable).
Hm, is there any reason not accepting multiple arguments?
I need to dig old es-discuss threads about it.
Comment 16 Yusuke Suzuki 2015-02-24 20:21:18 PST
Created attachment 247301 [details]
Patch
Comment 17 Yusuke Suzuki 2015-02-25 09:25:37 PST
Comment on attachment 247301 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247301&action=review

Added comments.

> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:29
> +    if (this == null || (typeof this !== "object" && typeof this !== "function"))

This is not efficient. Since there's bytecode `op_is_object` exists, using it here is the best way to generate guard for non-objects.
But now, we dont' have a way to generate a specific bytecode here.

I'm now planning to implement bytecode level intrinsic into priviledged JavaScript in a separate patch.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1938
> +        emitJumpIfTrue(emitIsObject(newTemporary(), src), isObjectLabel.get());

Simplified :)

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2641
> +    }

When an error is thrown in enumeration's body, we need to handle IteratorClose.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2654
> +    }

When `break` is executed, we need to handle IteratorClose.

> Source/JavaScriptCore/tests/stress/set-iterators-next.js:26
> +// TODO: Set.prototype.values() is not exposed.

Now values method is not exposed since it may hide some variables within with-scope.
@@unscopable will solve it. So after @@unscopable is implemented, we can expose values method and test it.
Comment 18 Yusuke Suzuki 2015-02-25 10:27:36 PST
Comment on attachment 247301 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247301&action=review

> Source/JavaScriptCore/runtime/JSArrayIterator.cpp:54
> +        itemKind = jsNontrivialString(&vm, ASCIILiteral("key+value"));

Is it preferrable to save these string in VM's smallStrings?
Comment 19 Yusuke Suzuki 2015-02-27 02:44:15 PST
Created attachment 247505 [details]
Patch
Comment 20 Yusuke Suzuki 2015-02-27 02:45:42 PST
Comment on attachment 247505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247505&action=review

Added comments

> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:55
> +    if (itemKind === @arrayIterationKindKey) {

Use numbers for kind representation instead of string. It's more efficient.
Comment 21 Yusuke Suzuki 2015-03-02 15:03:43 PST
Could anyone review this patch?
Comment 22 Filip Pizlo 2015-03-02 15:22:50 PST
Comment on attachment 247505 [details]
Patch

I'm looking at it...
Comment 23 Filip Pizlo 2015-03-02 15:37:31 PST
Comment on attachment 247505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247505&action=review

> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:32
> +function next() {
> +    "use strict";
> +
> +    if (this == null || (typeof this !== "object" && typeof this !== "function"))
> +        throw new @TypeError("%ArrayIteratorPrototype%.next requires that |this| be Object");
> +
> +    var itemKind = this.@arrayIterationKind;

This method is enormous and almost certainly not inlineable.  This will defeat object allocation sinking and a bunch of other optimizations.  Is it clear that all of these various checks are needed?

> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:39
> +            value: undefined,

You don't have to set value to anything if done is true.

> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:40
> +            done: true

You should add the done property before you add the value property.

> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:52
> +        return {
> +            value: undefined,
> +            done: true
> +        };
> +    };

For the purpose of object allocation sinking, it's much better if you create the result object using just one allocation.  I.e. this is better:

var o = {};
if (index >= length) {
    o.done = true;
} else {
    o.done = false;
    o.value = elementValue;
}
return o;

and this is worse:

if (index >= length) {
    return { done : true };
else {
    reutnr { done : false, value: elementValue };
}

Long term, we will fix the bug that makes the second form slower - but for now you might as well structure it to be easier to optimize.

> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:69
> +        value: [ index, elementValue ],

Is this really how this is supposed to work?  My understanding is that the value property is just supposed to contain the value, not an array with index and value.
Comment 24 Yusuke Suzuki 2015-03-02 16:24:10 PST
Comment on attachment 247505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247505&action=review

Thanks for your review!

>> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:32
>> +    var itemKind = this.@arrayIterationKind;
> 
> This method is enormous and almost certainly not inlineable.  This will defeat object allocation sinking and a bunch of other optimizations.  Is it clear that all of these various checks are needed?

The above check `if (this == null || (typeof this !== "object" && typeof this !== "function"))` is needed since we need to guarantee `this` is Object type (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%arrayiteratorprototype%.next).
But, I've come up with an idea. Instead of above check, we can  just use

if (this == null)
    throw new @TypeError(...);

var itemKind = this.@arrayIterationKind.
...


This is because since only array iterator instance has @arrayIterationKind property, all primitive values and non array-iterator objects can be filtered by this.

>> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:39
>> +            value: undefined,
> 
> You don't have to set value to anything if done is true.

However, since http://people.mozilla.org/~jorendorff/es6-draft.html#sec-createiterresultobject says that value shold be there, so we can observe this by executing the following script.

var array = [42];
var iter = array[Symbol.iterator]();
var result = iter.next();
var doneResult = iter.next();
assert(doneResult.hasOwnProperty('value'));

What do you think of?

>> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:40
>> +            done: true
> 
> You should add the done property before you add the value property.

Thanks! I'll follow.

>> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:52
>> +    };
> 
> For the purpose of object allocation sinking, it's much better if you create the result object using just one allocation.  I.e. this is better:
> 
> var o = {};
> if (index >= length) {
>     o.done = true;
> } else {
>     o.done = false;
>     o.value = elementValue;
> }
> return o;
> 
> and this is worse:
> 
> if (index >= length) {
>     return { done : true };
> else {
>     reutnr { done : false, value: elementValue };
> }
> 
> Long term, we will fix the bug that makes the second form slower - but for now you might as well structure it to be easier to optimize.

Thanks for your pointing! I'll fix it.

>> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:69
>> +        value: [ index, elementValue ],
> 
> Is this really how this is supposed to work?  My understanding is that the value property is just supposed to contain the value, not an array with index and value.

Reaching here, itemKind is @arrayIterationKindKeyValue. When iterator is this kind, it returnes [index, element] as value. (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%arrayiteratorprototype%.next step 17.b)
This is used (& exposed to users) when using array.entries();
It is intended to be used like the following,

for (var [index, value] of array.entries()) {
    print(index, value);
}
Comment 25 Yusuke Suzuki 2015-03-02 17:01:51 PST
Created attachment 247722 [details]
Patch
Comment 26 Filip Pizlo 2015-03-02 17:09:18 PST
(In reply to comment #24)
> Comment on attachment 247505 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247505&action=review
> 
> Thanks for your review!
> 
> >> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:32
> >> +    var itemKind = this.@arrayIterationKind;
> > 
> > This method is enormous and almost certainly not inlineable.  This will defeat object allocation sinking and a bunch of other optimizations.  Is it clear that all of these various checks are needed?
> 
> The above check `if (this == null || (typeof this !== "object" && typeof
> this !== "function"))` is needed since we need to guarantee `this` is Object
> type
> (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-
> %arrayiteratorprototype%.next).
> But, I've come up with an idea. Instead of above check, we can  just use
> 
> if (this == null)
>     throw new @TypeError(...);
> 
> var itemKind = this.@arrayIterationKind.
> ...
> 
> 
> This is because since only array iterator instance has @arrayIterationKind
> property, all primitive values and non array-iterator objects can be
> filtered by this.

So, how would you get "this" to be null?

> 
> >> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:39
> >> +            value: undefined,
> > 
> > You don't have to set value to anything if done is true.
> 
> However, since
> http://people.mozilla.org/~jorendorff/es6-draft.html#sec-
> createiterresultobject says that value shold be there, so we can observe
> this by executing the following script.
> 
> var array = [42];
> var iter = array[Symbol.iterator]();
> var result = iter.next();
> var doneResult = iter.next();
> assert(doneResult.hasOwnProperty('value'));
> 
> What do you think of?

Gotcha, I read an earlier version of the spec and was confused.

> 
> >> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:40
> >> +            done: true
> > 
> > You should add the done property before you add the value property.
> 
> Thanks! I'll follow.

I suggested that based on my assumption that "value" shouldn't be added if "done" is "true".  So, you can use whatever order you want.  But it's best to be consistent with what users tend to be doing, and I don't presume to know.

> 
> >> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:52
> >> +    };
> > 
> > For the purpose of object allocation sinking, it's much better if you create the result object using just one allocation.  I.e. this is better:
> > 
> > var o = {};
> > if (index >= length) {
> >     o.done = true;
> > } else {
> >     o.done = false;
> >     o.value = elementValue;
> > }
> > return o;
> > 
> > and this is worse:
> > 
> > if (index >= length) {
> >     return { done : true };
> > else {
> >     reutnr { done : false, value: elementValue };
> > }
> > 
> > Long term, we will fix the bug that makes the second form slower - but for now you might as well structure it to be easier to optimize.
> 
> Thanks for your pointing! I'll fix it.
> 
> >> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:69
> >> +        value: [ index, elementValue ],
> > 
> > Is this really how this is supposed to work?  My understanding is that the value property is just supposed to contain the value, not an array with index and value.
> 
> Reaching here, itemKind is @arrayIterationKindKeyValue. When iterator is
> this kind, it returnes [index, element] as value.
> (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-
> %arrayiteratorprototype%.next step 17.b)
> This is used (& exposed to users) when using array.entries();
> It is intended to be used like the following,
> 
> for (var [index, value] of array.entries()) {
>     print(index, value);
> }

I see.  I misunderstood.
Comment 27 Filip Pizlo 2015-03-02 17:10:09 PST
Comment on attachment 247722 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247722&action=review

> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:32
> +    var itemKind = this.@arrayIterationKind;

Is this really how it's supposed to work?  Is it possible to have different next methods depending on iteration kind?
Comment 28 Yusuke Suzuki 2015-03-02 17:47:34 PST
Comment on attachment 247505 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247505&action=review

>>>> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:32
>>>> +    var itemKind = this.@arrayIterationKind;
>>> 
>>> This method is enormous and almost certainly not inlineable.  This will defeat object allocation sinking and a bunch of other optimizations.  Is it clear that all of these various checks are needed?
>> 
>> The above check `if (this == null || (typeof this !== "object" && typeof this !== "function"))` is needed since we need to guarantee `this` is Object type (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%arrayiteratorprototype%.next).
>> But, I've come up with an idea. Instead of above check, we can  just use
>> 
>> if (this == null)
>>     throw new @TypeError(...);
>> 
>> var itemKind = this.@arrayIterationKind.
>> ...
>> 
>> 
>> This is because since only array iterator instance has @arrayIterationKind property, all primitive values and non array-iterator objects can be filtered by this.
> 
> So, how would you get "this" to be null?

We can extract this `next` method since it is now exposed to users.
So,

var array = [42];
var arrayIterator = array[Symbol.iterator]();
var arrayIteratorPrototype = arrayIterator.__proto__;
var arrayIteratorPrototypeNext = arrayIterator.next;

// When calling like the following, |this| becomes null. (since ArrayIterator.prototype.next is strict code, |this| doesn't become the global object.).
arrayIteratorPrototypeNext.call(null);

>>>> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:40
>>>> +            done: true
>>> 
>>> You should add the done property before you add the value property.
>> 
>> Thanks! I'll follow.
> 
> I suggested that based on my assumption that "value" shouldn't be added if "done" is "true".  So, you can use whatever order you want.  But it's best to be consistent with what users tend to be doing, and I don't presume to know.

Ah, I've got it.

>>>> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:52
>>>> +    };
>>> 
>>> For the purpose of object allocation sinking, it's much better if you create the result object using just one allocation.  I.e. this is better:
>>> 
>>> var o = {};
>>> if (index >= length) {
>>>     o.done = true;
>>> } else {
>>>     o.done = false;
>>>     o.value = elementValue;
>>> }
>>> return o;
>>> 
>>> and this is worse:
>>> 
>>> if (index >= length) {
>>>     return { done : true };
>>> else {
>>>     reutnr { done : false, value: elementValue };
>>> }
>>> 
>>> Long term, we will fix the bug that makes the second form slower - but for now you might as well structure it to be easier to optimize.
>> 
>> Thanks for your pointing! I'll fix it.
> 
> I see.  I misunderstood.

Oops! I missed that [[Put]].
When using the style,

var o = {};
o.value = 'value;

We can disturb it by defining accessors to Object.prototype.value...
So I suggest that extracting the object allocation function.

function createIteratorResult(done, value) {
    return {
        done: done,
        value: value
    };
}

And use it.
Comment 29 Yusuke Suzuki 2015-03-02 17:48:34 PST
Comment on attachment 247722 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247722&action=review

>> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:32
>> +    var itemKind = this.@arrayIterationKind;
> 
> Is this really how it's supposed to work?  Is it possible to have different next methods depending on iteration kind?

Since ArrayIterator.prototype.next is exposed to users and it is now defined in ArrayIteratorPrototype (not ArrayIterator object itself),
we can observe it when each iterator has a different next method.

var array = [42,42];
var iter1 = array.entries();
var iter2 = array.keys();

assert(iter1.next === iter2.next);
assert(iter1.__proto__ === iter2.__proto__);
assert(iter1.__proto__.hasOwnProperty('next'));
assert(!iter1.hasOwnProperty('next'));
assert(iter2.__proto__.hasOwnProperty('next'));
assert(!iter2.hasOwnProperty('next'));

But, defining functions for each array iteration kind sounds very nice. So what do you think of this style?

function next() {
    "use strict";
    if (this == null)
        throw new @TypeError("%ArrayIteratorPrototype%.next requires that |this| not be null or undefined");

    var nextForKind = this.@arrayNextForKind;
    if (!nextForKind)
        throw new @TypeError("%ArrayIteratorPrototype%.next requires that |this| be Array Iterator Instance");

    return this.@nextForKind();
}
Comment 30 Joseph Pecoraro 2015-03-02 18:06:13 PST
Comment on attachment 247722 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247722&action=review

> Source/JavaScriptCore/tests/stress/custom-iterators.js:1
> +// This test checks the behavior of custom iterable objects.

These tests are great! Should there also be basic LayoutTests alongside the JSC stress tests? Or is it more common for JSC tests to just be stress tests now?
Comment 31 Yusuke Suzuki 2015-03-02 18:17:56 PST
Comment on attachment 247722 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247722&action=review

>> Source/JavaScriptCore/tests/stress/custom-iterators.js:1
>> +// This test checks the behavior of custom iterable objects.
> 
> These tests are great! Should there also be basic LayoutTests alongside the JSC stress tests? Or is it more common for JSC tests to just be stress tests now?

Is it preferable to move this to layout-tests?
Or adding the same meaning tests to layout-tests additionally?
Comment 32 Filip Pizlo 2015-03-02 18:23:15 PST
(In reply to comment #31)
> Comment on attachment 247722 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247722&action=review
> 
> >> Source/JavaScriptCore/tests/stress/custom-iterators.js:1
> >> +// This test checks the behavior of custom iterable objects.
> > 
> > These tests are great! Should there also be basic LayoutTests alongside the JSC stress tests? Or is it more common for JSC tests to just be stress tests now?
> 
> Is it preferable to move this to layout-tests?
> Or adding the same meaning tests to layout-tests additionally?

It's preferable to just have JSC stress tests.  Layout tests don't do a good job of putting JSC through its various execution engine modes, and they are more cumbersome to write.

If it's possible to write it as a JSC stress test, then that's what you should do, and you shouldn't bother with a layout test.

The only exception is JSRegress benchmarks (i.e. LayoutTests/js/regress/script-tests), which double as benchmarks when running run-jsc-benchmarks.  But these are not strictly correctness tests.  These are short-running benchmarks - aimed to run for 10ms or less.  If possible, these benchmarks should check their results. Both run-webkit-tests and run-jsc-stress-tests will run them, and we sometimes catch bugs from them.
Comment 33 Yusuke Suzuki 2015-03-02 18:44:24 PST
Comment on attachment 247722 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247722&action=review

>>>> Source/JavaScriptCore/tests/stress/custom-iterators.js:1
>>>> +// This test checks the behavior of custom iterable objects.
>>> 
>>> These tests are great! Should there also be basic LayoutTests alongside the JSC stress tests? Or is it more common for JSC tests to just be stress tests now?
>> 
>> Is it preferable to move this to layout-tests?
>> Or adding the same meaning tests to layout-tests additionally?
> 
> It's preferable to just have JSC stress tests.  Layout tests don't do a good job of putting JSC through its various execution engine modes, and they are more cumbersome to write.
> 
> If it's possible to write it as a JSC stress test, then that's what you should do, and you shouldn't bother with a layout test.
> 
> The only exception is JSRegress benchmarks (i.e. LayoutTests/js/regress/script-tests), which double as benchmarks when running run-jsc-benchmarks.  But these are not strictly correctness tests.  These are short-running benchmarks - aimed to run for 10ms or less.  If possible, these benchmarks should check their results. Both run-webkit-tests and run-jsc-stress-tests will run them, and we sometimes catch bugs from them.

OK. Thank you for your clarification :D
Comment 34 Yusuke Suzuki 2015-03-02 18:53:04 PST
Comment on attachment 247722 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247722&action=review

>>> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:32
>>> +    var itemKind = this.@arrayIterationKind;
>> 
>> Is this really how it's supposed to work?  Is it possible to have different next methods depending on iteration kind?
> 
> Since ArrayIterator.prototype.next is exposed to users and it is now defined in ArrayIteratorPrototype (not ArrayIterator object itself),
> we can observe it when each iterator has a different next method.
> 
> var array = [42,42];
> var iter1 = array.entries();
> var iter2 = array.keys();
> 
> assert(iter1.next === iter2.next);
> assert(iter1.__proto__ === iter2.__proto__);
> assert(iter1.__proto__.hasOwnProperty('next'));
> assert(!iter1.hasOwnProperty('next'));
> assert(iter2.__proto__.hasOwnProperty('next'));
> assert(!iter2.hasOwnProperty('next'));
> 
> But, defining functions for each array iteration kind sounds very nice. So what do you think of this style?
> 
> function next() {
>     "use strict";
>     if (this == null)
>         throw new @TypeError("%ArrayIteratorPrototype%.next requires that |this| not be null or undefined");
> 
>     var nextForKind = this.@arrayNextForKind;
>     if (!nextForKind)
>         throw new @TypeError("%ArrayIteratorPrototype%.next requires that |this| be Array Iterator Instance");
> 
>     return this.@nextForKind();
> }

Ah, but, if we extract object allocation to createIteratorResult function, the body becomes fairly simple.
Only the following code depends on the iteration kind.

    if (itemKind === @arrayIterationKindKey)
        return createIteratorResult(index, false);

    var elementValue = array[index];
    if (itemKind === @arrayIterationKindValue)
        return createIteratorResult(elementValue, false);

     // itemKind is @arrayIteratorKindKeyValue.
    return createIteratorResult([ index, elementValue ], false);

So maybe, spliting it into 3 functions is not necessary I think. What do you think of it?
Comment 35 Yusuke Suzuki 2015-03-02 21:17:40 PST
Created attachment 247746 [details]
Patch
Comment 36 Yusuke Suzuki 2015-03-02 21:22:13 PST
Comment on attachment 247746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247746&action=review

Uploaded the revised patch.

> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:56
> +    }

After considering the implementation, I changed the form to the current one.
1. Object allocation uses ObjectLiteral. It uses [[DefineOwnProperty]] and accessors in Object.prototype can't disturb it.
2. Since array object extraction (L39), index extraction (L41), length  check (L43), index update (L46) are duplicate in all item kinds, in the meantime, we don't split method into 3 (key, value, key+value).

What do you think of?

> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:61
> +    };

Object allocations are now aggregated here.
Comment 37 Yusuke Suzuki 2015-03-02 22:49:13 PST
Comment on attachment 247722 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247722&action=review

>>>> Source/JavaScriptCore/builtins/ArrayIterator.prototype.js:32
>>>> +    var itemKind = this.@arrayIterationKind;
>>> 
>>> Is this really how it's supposed to work?  Is it possible to have different next methods depending on iteration kind?
>> 
>> Since ArrayIterator.prototype.next is exposed to users and it is now defined in ArrayIteratorPrototype (not ArrayIterator object itself),
>> we can observe it when each iterator has a different next method.
>> 
>> var array = [42,42];
>> var iter1 = array.entries();
>> var iter2 = array.keys();
>> 
>> assert(iter1.next === iter2.next);
>> assert(iter1.__proto__ === iter2.__proto__);
>> assert(iter1.__proto__.hasOwnProperty('next'));
>> assert(!iter1.hasOwnProperty('next'));
>> assert(iter2.__proto__.hasOwnProperty('next'));
>> assert(!iter2.hasOwnProperty('next'));
>> 
>> But, defining functions for each array iteration kind sounds very nice. So what do you think of this style?
>> 
>> function next() {
>>     "use strict";
>>     if (this == null)
>>         throw new @TypeError("%ArrayIteratorPrototype%.next requires that |this| not be null or undefined");
>> 
>>     var nextForKind = this.@arrayNextForKind;
>>     if (!nextForKind)
>>         throw new @TypeError("%ArrayIteratorPrototype%.next requires that |this| be Array Iterator Instance");
>> 
>>     return this.@nextForKind();
>> }
> 
> Ah, but, if we extract object allocation to createIteratorResult function, the body becomes fairly simple.
> Only the following code depends on the iteration kind.
> 
>     if (itemKind === @arrayIterationKindKey)
>         return createIteratorResult(index, false);
> 
>     var elementValue = array[index];
>     if (itemKind === @arrayIterationKindValue)
>         return createIteratorResult(elementValue, false);
> 
>      // itemKind is @arrayIteratorKindKeyValue.
>     return createIteratorResult([ index, elementValue ], false);
> 
> So maybe, spliting it into 3 functions is not necessary I think. What do you think of it?

"> Is this really how it's supposed to work?"

Yes, This itemKind represents the ArrayIterator's kind, that is "key", "value" and "key+value".
(http://people.mozilla.org/~jorendorff/es6-draft.html#sec-properties-of-array-iterator-instances)

When "key", the iterator produces keys (in array, it's index). When "value", the iterator produces values (in array, it's element).
And when "key+value", the iterator produces the pair of key and value as [key, value].

To make array iterator efficient, instead of strings that represents kind (like "key", "value", "key+value"), we use small integers, (0, 1, 2).
It's defined as enum in C++ (ArrayIterationKind) and it's also exposed to priviledged JS code with private symbols (global.{@arrayIterationKindKey, @arrayIterationKindValue and @arrayIterationKindKeyValue})
Comment 38 Yusuke Suzuki 2015-03-03 18:39:36 PST
OK, updated the patch. Could you take a look?
Comment 39 Yusuke Suzuki 2015-03-04 19:47:10 PST
Comment on attachment 247746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247746&action=review

And added comments for minor fixes.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2574
> +    RefPtr<RegisterID> next = emitGetById(newTemporary(), iterator.get(), propertyNames().next);

Oops. it should be moved into the following braces.
Comment 40 Yusuke Suzuki 2015-03-04 20:08:51 PST
Comment on attachment 247746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247746&action=review

> Source/JavaScriptCore/runtime/JSPromiseConstructor.cpp:346
> +static JSValue performPromiseAll(ExecState* exec, JSValue iterator, JSValue C, JSPromiseDeferred* deferred)

And one comment is that, currently, the change in Promise.all / Promise.race is limited in only iterator treatment things. (Use IteratorOperations)

This is because, from the old spec, Promise.all and Promise.race architecture is largely changed. (e.g. changing [[CountDown]] to remainingElementsCount)
So updating it seems another concern. I think performing fundamental changes in the subsequent patch like "Upgrading ES6 Promises) is better.
Comment 41 Filip Pizlo 2015-03-04 20:09:34 PST
Comment on attachment 247746 [details]
Patch

I buy this.  LGTM. :-)
Comment 42 Yusuke Suzuki 2015-03-05 06:57:56 PST
Committed r181077: <http://trac.webkit.org/changeset/181077>
Comment 43 Csaba Osztrogonác 2015-03-11 05:03:40 PDT
(In reply to comment #42)
> Committed r181077: <http://trac.webkit.org/changeset/181077>

It caused a regression on AArch64 (at least on Linux) - bug142575 .