Bug 141351

In the JSC's iterator interface, using [[iteratorNext]] for next() method. In the latest spec, it is exposed as Iterator.prototype.next()

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.

And this https://bugs.webkit.org/show_bug.cgi?id=122575

Created attachment 246467 [details] prototype rev1 prototype

Created attachment 246489 [details] prototype rev2 prototype

Created attachment 246500 [details] prototype rev3 prototype

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.

(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.

Created attachment 246730 [details] prototype rev4 prototype

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

(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.

(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 :)

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

(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.

Created attachment 247301 [details] Patch

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

Created attachment 247505 [details] Patch

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.

Could anyone review this patch?

Comment on attachment 247505 [details] Patch I'm looking at it...

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 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); }

Created attachment 247722 [details] Patch

(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 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 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 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 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 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?

(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 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 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?

Created attachment 247746 [details] Patch

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 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})

OK, updated the patch. Could you take a look?

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 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 on attachment 247746 [details] Patch I buy this. LGTM. :-)

Committed r181077: <http://trac.webkit.org/changeset/181077>

(In reply to comment #42) > Committed r181077: <http://trac.webkit.org/changeset/181077> It caused a regression on AArch64 (at least on Linux) - bug142575 .