Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary step on %ArrayIteratorPrototype%.next #412

Closed
wants to merge 1 commit into from

Conversation

leobalter
Copy link
Member

It is not necessary to account for TypedArrays but making this special case.

This change enables runtime optimization on the method call. The api is
already secured for any further property access on a TypedArray instance if
a custom length is defined.

It is not necessary to account for TypedArrays but making this special case.

This change enables runtime optimization on the method call. The api is
already secured for any further property access on a TypedArray instance if
a custom length is defined.
@anba
Copy link
Contributor

anba commented Feb 25, 2016

The extra step was added to fix https://bugs.ecmascript.org/show_bug.cgi?id=3269. Is there any evidence we no longer want/need consistent access to the underlying typed array length for all built-ins?

@leobalter
Copy link
Member Author

on the mentioned link, the toLocaleString is already specialized.

I believe the result will be fine on the iterators if a custom "length" is defined. There should a purpose for that. Other alternative is tell the iterator access the [[ArrayLength]] internal slot in place of performing a [[Get]] of "length". As in the other methods WRT such optimization must not introduce any observable changes in the specified behavior of the algorithm.

@anba
Copy link
Contributor

anba commented Feb 25, 2016

on the mentioned link, the toLocaleString is already specialized.

Yes, toLocaleString was specialized in response to that issue report.

I believe the result will be fine on the iterators if a custom "length" is defined.

It depends on how much consistent behaviour we'd like to have for operations on typed arrays. For example when we compare %TypedArray%.prototype.map and %TypedArray%.from:

// Typed array with own "length" property
var ta = Object.defineProperty(new Int8Array([1,2,3]), "length", {
  writable: true, configurable: true, value: 1
});

// Prints "1,4,9"
print(ta.map(v => v * v).toString());

// Prints "1,4,9" in ES2015, but only "1" with the proposed change to %ArrayIteratorPrototype%.next
print(Int8Array.from(ta, v => v * v).toString());

Other alternative is tell the iterator access the [[ArrayLength]] internal slot in place of performing a [[Get]] of "length".

That's the behaviour in ES2015: Instead of performing ToLength(Get(a, "length")), the [[ArrayLength]] internal slot is directly accessed.

@littledan
Copy link
Member

I like Leo's proposal, but I would also be fine with giving TypedArrays a separate iterator from Arrays. However, it is not trivial to optimize away the extra conditional that the current unification-plus-specialization has. It seems like the use of [[ArrayLength]] is supposed to be an optimization, but it actually creates an extra challenges for implementors.

@leobalter
Copy link
Member Author

I would also be fine with giving TypedArrays a separate iterator from Arrays

I believe reusing %ArrayIteratorPrototype% is the best option here. The odds for getting length instead of [[ArrayLength]] fits the goal for performance as the latter is better for other instance methods on TypedArrays.

If necessary I can change this and adapt to what works better.

@littledan
Copy link
Member

@allenwb What do you think of this proposal?

@allenwb
Copy link
Member

allenwb commented Mar 15, 2016

I don't like this proposed change. Typed Array methods should all consistently use the same definition of "length". Arguably, that definition could be "Get(tarr, 'length')" but the current use of [[ArrayLength]] was selected in order to make it easier to statically optimized Typed Array specialized versions of the Array prototype methods. Consistently using "Get(tarr, 'length')" would eliminate the opportunity for that optimization.

Arrays and Typed Arrays share the use of %ArrayIteratorPrototype% so any properties added tp %ArrayInteratorPrototype% are also visible to iterators over Typed Array instances. That decision was made before https://bugs.ecmascript.org/show_bug.cgi?id=3269 , so prior to fixing bug 3269 was exactly as now proposed by @leobalter .

Another alternative would be to make %TypedArrayIterator% a subclass of %ArrayIterator% that overrides the next method. However, we choose to avoid do that sort of inheritance in the ES6 spec. and we would have to think about whether we want to change that design rule.

But, I'm inclined to favor leaving things as current specified as and letting implementation figure out how to optimize it. Note that the determination of whether the iterator object is an Array-like or a TypedArray can be made within CreateArrayIterator. So, an implementation might add an implementation specific internal slot named [[nextSpecialization]] for Array iterator instances and initialize within CreateArrayIterator it to statically constructed specializations of next. In that case the implementation of %TypedArrayPrototype% next might be something that was approximately equivalent to:

next() {
   return this.[[nextSpecialization]]()
}

This seems like the sort of specialization that the hotspot optimizers of jits should be able to inline or otherwise optimize. If it isn't happening in hot code then the cost of dynamic Array/TypedArray discrimination (however it is done) probably doesn't matter.

@leobalter
Copy link
Member Author

So, an implementation might add an implementation specific internal slot named [[nextSpecialization]] for Array iterator instances and initialize within CreateArrayIterator it to statically constructed specializations of next

This is a very interesting approach. We can close this and I might work on this optimization or any other conclusion.

@bterlson bterlson closed this Mar 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants