Access the .next method once, at the beginning, of the iteration protocol
#988
Conversation
kmiller68
added
the
needs consensus
|
I do think it needs to be a full proposal. It will certainly stimulate the level of discussion we'd expect of a full proposal. |
spec.html
Outdated
| @@ -4681,19 +4681,21 @@ <h1 id="ao-issuperreference">IsSuperReference ( _V_ )</h1> | |||
| 1. Set _method_ to ? GetMethod(_obj_, @@iterator). | |||
| 1. Let _iterator_ be ? Call(_method_, _obj_). | |||
| 1. If Type(_iterator_) is not Object, throw a *TypeError* exception. | |||
| 1. Return _iterator_. | |||
| 1. Let _nextMethod_ be ? GetV(_iterator_, `"next"`). | |||
There was a problem hiding this comment.
Should this use GetMethod instead?
There was a problem hiding this comment.
I left the check at the call site to avoid changing more than what was needed. I'd be happy to make that change if that's the preferred behavior, however.
There was a problem hiding this comment.
In order to find precedents, you may look at the various Array methods that accept a callback as argument (.every, .forEach, .map, .sort, etc.), or at the Promise constructor and its methods. The check for callable is an early check.
There was a problem hiding this comment.
Fair enough, I'll change.
There was a problem hiding this comment.
I don't think this should use GetMethod.
"next" isn't an optional method, so treating "null" specially isn't really required, and since the method isn't optional, the only real impact of it would be the error message saying "next" is undefined for an iterator like { next: null }.
... Also, it's a slight semantic change, since ToT IteratorNext just uses Invoke, which also uses GetV.
The IsCallable test is also only very, very slightly earlier than it would otherwise be with this change. It's hard to observe the difference. When the iterator protocol is used, IteratorStep is basically always the next step after GetIterator. Destructuring assignment with an empty ArrayAssignmentPatterns seems to be the only case where it happens differently, and in those cases, the callable-ness of "next" is irrelevant.
There was a problem hiding this comment.
I think you're right @caitp, we shouldn't use GetMethod. I was under the impression that GetMethod returned a either a function or an exception. It seems to me that we should either do If IsCallable(nextMethod) is false, throw a TypeError exception. after the GetV or just wait until the the exception is thrown when trying to call nextMethod. I'm slightly more partial to the latter since it's a little less spec text and might be a little nicer for implementations.
By a "full proposal", do you think what we've been doing for needs-consensus pull requests would be enough, with requiring test262 tests? Maybe if we also got implementation feedback before merging? |
rwaldron
added
has consensus
tc39/ecma262#988 changes the iteration protocol such that the "next" method is only loaded from the iterator object once during the prologue of iteration, rather than during each step.
tc39/ecma262#988 changes the iteration protocol such that the "next" method is only loaded from the iterator object once during the prologue of iteration, rather than during each step.
tc39/ecma262#988 changes the iteration protocol such that the "next" method is only loaded from the iterator object once during the prologue of iteration, rather than during each step.
|
we have updated tests, thanks @caitp! |
leobalter
removed
the
needs test262 tests
Update behaviour based on changes from tc39/ecma262#988. The actual spec change PR for async iteration is not yet uploaded. This does not include any changes to Async-from-Sync Iterator.
Update behaviour based on changes from tc39/ecma262#988. The actual spec change PR for async iteration is not yet uploaded. This does not include any changes to Async-from-Sync Iterator.
spec.html
Outdated
| <h1>IteratorStep ( _iterator_ )</h1> | ||
| <p>The abstract operation IteratorStep with argument _iterator_ requests the next value from _iterator_ and returns either *false* indicating that the iterator has reached its end or the IteratorResult object if a next value is available. IteratorStep performs the following steps:</p> | ||
| <h1>IteratorStep ( _iteratorRecord_ )</h1> | ||
| <p>The abstract operation IteratorStep with argument _iteratorRecord_ requests the next value from _iteratorRecord_.[[Iterator]] by calling _iteratorRecord_.[[NextFunction]] and returns either *false* indicating that the iterator has reached its end or the IteratorResult object if a next value is available. IteratorStep performs the following steps:</p> |
There was a problem hiding this comment.
Presumably should be [[NextMethod]].
|
Hmm, I attempted to push commits to @kmiller68's master branch but accidentally pushed the current master over which closed the PR and doesn't let me push further commits. Very strange. Anyway, still working on fixing up a few minor mistakes here and will push soon. |
|
This is continued at #1021 (since I can't reopen this one myself). |
This change updates test cases to assume that the "next" method is only loaded from a synchronous iterator once, and is re-used for each call to Async-from-Sync Iterator.next(), based on tc39/ecma262#988
|
What are the differences in code? |
tc39/ecma262#988 gained concensus during the september 2017 TC39 meetings. This moves the load of the "next" method to the very beginning of the iteration protocol, rather than during each iteration step. This impacts: - yield* - for-of loops - spread arguments - array spreads In the v8 implementation, this also affects async iteration versions of these things (the sole exception being the Async-From-Sync iterator, which requires a few more changes to work with this, likely done in a followup patch). This change introduces a new AST node, ResolvedProperty, which can be used as a callee by Call nodes to produce the same bytecode as Property calls, without observably re-loading the property. This is used in several AST-desugarings involving the iteration protocol. BUG=v8:6861, v8:5699 [email protected], [email protected], [email protected] Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng Change-Id: Ib81106a0182687fc5efea0bc32302ad06376773b Reviewed-on: https://chromium-review.googlesource.com/687997 Commit-Queue: Caitlin Potter <[email protected]> Reviewed-by: Ross McIlroy <[email protected]> Reviewed-by: Adam Klein <[email protected]> Reviewed-by: Georg Neis <[email protected]> Cr-Commit-Position: refs/heads/master@{#50452}
…tion" This reverts commit bf4cc9e. Reason for revert: Breaks windows with msvc and linux with gcc https://build.chromium.org/p/client.v8/builders/V8%20Win64%20-%20msvc/builds/841 https://build.chromium.org/p/client.v8/builders/V8%20Linux%20gcc%204.8/builds/17265 Original change's description: > [esnext] load `iterator.next` only once at beginning of iteration > > tc39/ecma262#988 gained concensus during the > september 2017 TC39 meetings. This moves the load of the "next" method > to the very beginning of the iteration protocol, rather than during > each iteration step. > > This impacts: > > - yield* > - for-of loops > - spread arguments > - array spreads > > In the v8 implementation, this also affects async iteration versions of > these things (the sole exception being the Async-From-Sync iterator, > which requires a few more changes to work with this, likely done in a > followup patch). > > This change introduces a new AST node, ResolvedProperty, which can be used > as a callee by Call nodes to produce the same bytecode as Property calls, > without observably re-loading the property. This is used in several > AST-desugarings involving the iteration protocol. > > BUG=v8:6861, v8:5699 > R=[email protected], [email protected], [email protected] > > Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng > Change-Id: Ib81106a0182687fc5efea0bc32302ad06376773b > Reviewed-on: https://chromium-review.googlesource.com/687997 > Commit-Queue: Caitlin Potter <[email protected]> > Reviewed-by: Ross McIlroy <[email protected]> > Reviewed-by: Adam Klein <[email protected]> > Reviewed-by: Georg Neis <[email protected]> > Cr-Commit-Position: refs/heads/master@{#50452} [email protected],[email protected],[email protected],[email protected],[email protected] Change-Id: I1797c0d596dfd6850d6f0f505f591a7a990dd1f1 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: v8:6861, v8:5699 Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng Reviewed-on: https://chromium-review.googlesource.com/857616 Reviewed-by: Michael Achenbach <[email protected]> Commit-Queue: Michael Achenbach <[email protected]> Cr-Commit-Position: refs/heads/master@{#50454}
…tion" tc39/ecma262#988 gained concensus during the september 2017 TC39 meetings. This moves the load of the "next" method to the very beginning of the iteration protocol, rather than during each iteration step. This impacts: - yield* - for-of loops - spread arguments - array spreads In the v8 implementation, this also affects async iteration versions of these things (the sole exception being the Async-From-Sync iterator, which requires a few more changes to work with this, likely done in a followup patch). This change introduces a new AST node, ResolvedProperty, which can be used as a callee by Call nodes to produce the same bytecode as Property calls, without observably re-loading the property. This is used in several AST-desugarings involving the iteration protocol. BUG=v8:6861, v8:5699 [email protected] [email protected], [email protected] Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng Change-Id: I9685db6e85315ba8a2df87a4537c2bf491e1e35b Reviewed-on: https://chromium-review.googlesource.com/857593 Commit-Queue: Caitlin Potter <[email protected]> Reviewed-by: Ross McIlroy <[email protected]> Cr-Commit-Position: refs/heads/master@{#50518}
A version of the spec change from tc39/ecma262#988, but applied to the Async-from-Sync iterator type. This change does not modify generated bytecode (but maybe it should to take advantage of load IC feedback for loading "next"). Doing this grows bytecode by quite a bit, since it's necessary to throw-if-not-an-object before loading "next" (which currently gets to live in a code stub instead). BUG=v8:5855 Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng Change-Id: I0d2affef664d1069b24c54a553d62e17b49e5a16 Reviewed-on: https://chromium-review.googlesource.com/723136 Commit-Queue: Caitlin Potter <[email protected]> Reviewed-by: Ross McIlroy <[email protected]> Reviewed-by: Jakob Gruber <[email protected]> Cr-Commit-Position: refs/heads/master@{#51078}
As discussed in this issue, this PR changes the how iteration works in the spec. Previously, the next method would be fetched off the iterator object on each iteration. This PR changes that to happen exactly once at the start of the protocol.
Some places in the spec used to pass a Record around with the iterator and the completion status and others just pass the iterator object. This has been simplified to make every point of iteration pass a Record containing the iterator, completion status, and now the next method.
Right now I made this a needs consensus PR, let me know if I should convert this to a proposal. This PR also relies on a compatibility check, although I expect that will not be an issue.