Simplify ArrayIterator to a cached length and a position counter - #21598
NullVoxPopuli wants to merge 3 commits into
Conversation
Split out of nvp/simplify-some-vm-hot-paths. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
| let memo = ++this.pos; | ||
|
|
||
| let { keyFor } = this; | ||
| if (memo >= this.length) return null; |
There was a problem hiding this comment.
will this also need to account for live array mutations during render?
-if (memo >= this.length) return null;
+if (memo >= this.iterator.length) return null;See johanrd#43
There was a problem hiding this comment.
ideally, this should error I think -- if the thing being iterated over was a tracked collection, it certainly would error
There was a problem hiding this comment.
Yes, tracked collections error (on both main and this PR), but not plain arrays (renders items backed by undefined)
What about:
if (DEBUG && this.length !== this.iterator.length) {
throw new Error(`the array was mutated while {{#each}} was iterating it`);
}
if (memo >= this.iterator.length) return null;|
very cool work! |
Logs how many blocks render when the backing array is truncated by user code that runs during iteration. On emberjs#21598. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8ja8hgWMpTns6ezSXEMzr
Logs how many blocks render when the backing array is truncated by user code that runs during iteration. On the emberjs#21598 base. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8ja8hgWMpTns6ezSXEMzr
Code in the block body or in a `key` getter runs between two `next()` calls. That code can change the length of a plain array, and a plain array has no tag. On main, `next()` compares against the live length: - a shorter array stops the iteration - a longer array renders the new items A stored length rendered blocks for `undefined` after a truncation, and skipped added items. The constructor still reads `length`, because only that read runs in the tracking frame of the iterator reference. It now stores the empty state only, as main does. Reported by johanrd in emberjs#21598. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* Read the array length in the ArrayIterator constructor
The iterator reference computes its value in a tracking frame.
The constructor is the only ArrayIterator code that runs in that frame.
`isEmpty()` and `next()` run later, in the frame of the VM.
Without a read in the constructor, the reference never consumes the tag of a tracked array.
The reference then returns the same iterator after a change, and the list block skips its sync.
This broke `{{each}} works when updating old items` for `trackedArray`.
It also stopped the list updates that the benchmark app waits for.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* Compare the rendered item count in the {{each}} reactivity tests
The assertion looped over the rendered elements only.
A list that did not render a new item passed the test.
`{{each}} works with new items` passed for `trackedArray` while the list did not update after `push`.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* Read the live array length in ArrayIterator#next
Code in the block body or in a `key` getter runs between two `next()` calls.
That code can change the length of a plain array, and a plain array has no tag.
On main, `next()` compares against the live length:
- a shorter array stops the iteration
- a longer array renders the new items
A stored length rendered blocks for `undefined` after a truncation, and skipped added items.
The constructor still reads `length`, because only that read runs in the tracking frame of the iterator reference.
It now stores the empty state only, as main does.
Reported by johanrd in emberjs#21598.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
---------
Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
these tests preserve accidentally existing behavior (found by @johanrd )
There was a problem hiding this comment.
unrelated cleanup for easier debugging with breakpoints
| } else { | ||
| this.current = { kind: 'first', value: iterator[this.pos] }; | ||
| } | ||
| // Only this read runs in the tracking frame of the iterator reference. |
There was a problem hiding this comment.
this is kinda "womp"
|
|
||
| let { keyFor } = this; | ||
| // The length is live because code in the block can change a plain array. | ||
| if (memo >= this.iterator.length) return null; |
There was a problem hiding this comment.
I think this implicit behavior we had here (and continue to have here): entangling with length, if it's tracked, could be why some of our iteration/updates is slow.
I need to think about how we can have a better list update/create/destroy algo in general, but that'll be a much bigger PR
Drops the
currentstate object thatArrayIteratorallocated per list and tracks progress with a position counter and a cached length instead.The length is still read in the constructor so a tracked array's tag is consumed inside the iterator reference's tracking frame.