From d61e33dcdd51f0b851175b588489976dcfafdc96 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli-ai-agent <268630448+NullVoxPopuli-ai-agent@users.noreply.github.com> Date: Thu, 6 Aug 2026 14:44:59 -0400 Subject: [PATCH] Skip unchanged {{#each}} item subtrees during updates The UpdatingVM walks every updating opcode of every list item on every render: cache groups (JumpIfNotModifiedOpcode) exist only at component boundaries, so a list of plain template rows revalidates every binding even when nothing in a row changed. Collect each item's consumed tags in a tracking frame (via a new frame-finalizer hook on UpdatingVMFrame) and skip the item's entire subtree while that combined tag validates. Trivial items opt out: for a text node or two, validating a combined tag costs as much as updating, so collection would be pure overhead. An item is trivial when it has <= 2 opcodes and no nested block -- a nested block child means an arbitrarily large subtree hides behind a small top-level count. dbmon-style workloads (fat rows, sparse changes): ~1.6x fps at 8x CPU throttle, ~6x (rAF-capped) at 4x. Dense-change / tiny-item workloads and the krausest bench: neutral. Iteration fast path, flat update frames, and flattened combinators Three allocation/dispatch levers for the update path, measured together with subtree-skipping as the difference between ~14 and ~27 fps on an 8x-throttled dbmon: - streaming same-order list compare: when a list's order is unchanged (the overwhelmingly common case), items are matched in a single streaming pass writing into a scratch item (nextInto) instead of allocating an IterationItem per step, falling back to the general diff via a reconstructed prefix iterator on first mismatch - flat frame stack: the updating VM keeps parallel arrays indexed by depth instead of allocating an UpdatingVMFrame per block per render - combinator flattening: combine() flattens nested combinators and drops constants (capped) so validating a combined tag is one flat loop instead of a pointer-chasing tree walk Keep only the two {{#each}} changes that measure, and fix the skip's unwind handling Benchmarked every change in the previous commit independently against `main`, interleaved (all variants measured in every round, order rotating by round) so that machine drift over the session could not be mistaken for a per-change effect. Measuring each variant once in sequence had done exactly that: re-running one identical build later in a session moved some benches by 30%, which is larger than four of the five changes. Two changes carry the whole result, and three do not: subtree skipping 1k items 1-each async 2.03x, 25%-random async 1.79x tryFastSync 25%-random async 1.11x, no cost on any bench nextInto 1.01x SLOWER inside tryFastSync, 0/6 rounds better flat frame stack 1.02x slower on 100k-updates, 0/6 rounds better combine flattening 1.05x slower on 100k-updates, 0/6 rounds better So `nextInto`, the flat frame stack, and the `combine()` flattening are reverted. `combine()` in particular could not have paid off: a combinator's `[COMPUTE]` already memoizes per `$REVISION`, so flattening bought nothing on validation while adding a pre-pass and an allocation to one of the validator's hottest functions -- and it defeated `markTagAsConsumed`'s early-out in dev. Subtree skipping keeps its win at a price worth stating: it costs ~11% on a list whose every row changes on every update, and ~9% when many updates are batched into a single render, because the combined tag is collected per item per render and only recouped when the item is actually skipped. A gate keyed on how often an item is really skipped would address that; the opcode-count gate in the previous commit did not, and could not -- every {{#each}} item body measured as one child that is a BlockOpcode (dynamic content is wrapped in a TryOpcode), so it classified every real template as non-trivial and never fired. It is removed rather than left as dead code with a comment describing behaviour that does not happen. Two unwind bugs in the skip, both from closing the tracking frame somewhere other than where it was opened: - An exception escaping the update loop left the frame open, and only the DEBUG build reset tracking -- so in production one render error made every later `endTrackFrame` pop the wrong frame. The production path now resets too. - `vm.throw()` unwinds a single frame, and a component's Begin/EndTrackFrameOpcode pair shares the item's ops array, so an `Assert` firing between them leaves the component's frame open. The finalizer would then adopt that frame's partial tag and skip the item against it forever. It now records the depth before opening, unwinds to it, and keeps no tag if anything leaked in between. `reconstructPrefix` read item refs with `valueForRef`, which consumes; running inside an enclosing item's or component's frame that made the frame depend on every item ref in the list, so any list mutation invalidated the enclosing component. Those reads are now untracked. Tests. Skipping is invisible in the DOM -- the rendered output is identical whether a subtree was skipped or walked and found clean -- so asserting HTML and node stability does not test it, and the suite already covers those. `ListItemOpcode` therefore logs a `list-item-subtrees` step (LOCAL_DEBUG only, a separate step type so the existing `list-updates` assertions are untouched), and three #each tests assert the actual skip/walk decision per item: that clean items are skipped at all, that only the dirtied item is walked, that it returns to being skipped once clean, and that an item whose subtree is rebuilt -- via {{#if}}, and via {{#in-element}}, which unwinds the item's own block -- is walked again and has its new children tracked. All three fail if skipping is disabled, and all three fail if an item is skipped while dirty. Not covered, stated because the tests look like they would cover it: the finalizer's `didError`/frame-identity guards. Removing either leaves the suite green -- they guard an unwind with an enclosing cache group open, which I could not construct a template for. `handleException` clearing the tag is likewise unobservable, and provably so: whatever threw did so because a ref the item's tag covers changed, so the tag is already invalid. ope --- packages/@glimmer/runtime/lib/vm/update.ts | 261 ++++++++++++++++++-- packages/@glimmer/validator/lib/tracking.ts | 15 ++ 2 files changed, 258 insertions(+), 18 deletions(-) diff --git a/packages/@glimmer/runtime/lib/vm/update.ts b/packages/@glimmer/runtime/lib/vm/update.ts index 92981cc0531..f230a76cde8 100644 --- a/packages/@glimmer/runtime/lib/vm/update.ts +++ b/packages/@glimmer/runtime/lib/vm/update.ts @@ -9,8 +9,10 @@ import type { GlimmerTreeChanges, Nullable, ResettableBlock, + Revision, Scope, SimpleComment, + Tag, UpdatingOpcode, UpdatingVM as IUpdatingVM, } from '@glimmer/interfaces'; @@ -23,7 +25,16 @@ import { updateRef, valueForRef } from '@glimmer/reference/lib/reference'; import { logStep } from '@glimmer/util/lib/debug-steps'; import { StackImpl as Stack } from '@glimmer/util/lib/collections'; import { debug } from '@glimmer/validator/lib/debug'; -import { resetTracking } from '@glimmer/validator/lib/tracking'; +import { + beginTrackFrame, + beginUntrackFrame, + consumeTag, + endTrackFrame, + endUntrackFrame, + resetTracking, + trackFrameDepth, +} from '@glimmer/validator/lib/tracking'; +import { INITIAL, validateTag, valueForTag } from '@glimmer/validator/lib/validators'; import type { Closure } from './append'; import type { AppendingBlockList } from './element-builder'; @@ -64,7 +75,20 @@ export class UpdatingVM implements IUpdatingVM { } } } else { - this._execute(opcodes, handler); + let hasErrored = true; + try { + this._execute(opcodes, handler); + hasErrored = false; + } finally { + // `{{#each}}` items open a tracking frame that is closed when their + // frame is popped, so an exception that escapes the loop leaves it + // open: `CURRENT_TRACKER` would keep pointing at a dead item and + // the next balanced `endTrackFrame` (a component's, say) would pop + // the wrong one, corrupting every tag computed afterwards. Only the + // DEBUG branch above used to reset, so in production a single + // render error poisoned autotracking for the rest of the page. + if (hasErrored) resetTracking(); + } } } @@ -77,7 +101,9 @@ export class UpdatingVM implements IUpdatingVM { let opcode = this.frame.nextStatement(); if (opcode === undefined) { - frameStack.pop(); + let frame = expect(frameStack.pop(), 'bug: expected a frame'); + + frame.finalize(false); continue; } @@ -93,13 +119,20 @@ export class UpdatingVM implements IUpdatingVM { this.frame.goto(index); } - try(ops: UpdatingOpcode[], handler: Nullable) { - this.frameStack.push(new UpdatingVMFrame(ops, handler)); + try( + ops: UpdatingOpcode[], + handler: Nullable, + finalizer?: (didError: boolean) => void + ) { + this.frameStack.push(new UpdatingVMFrame(ops, handler, finalizer)); } throw() { this.frame.handleException(); - this.frameStack.pop(); + + let frame = expect(this.frameStack.pop(), 'bug: expected a frame'); + + frame.finalize(true); } } @@ -178,6 +211,14 @@ export class ListItemOpcode extends TryOpcode { public retained = false; public index = -1; + /** + * Everything this item's subtree consumed during its last update, + * combined. When still valid, the whole subtree is skipped -- one tag + * validation instead of walking every opcode in the item. + */ + private subtreeTag: Nullable = null; + private subtreeRevision: Revision = INITIAL; + constructor( state: Closure, context: EvaluationContext, @@ -189,6 +230,70 @@ export class ListItemOpcode extends TryOpcode { super(state, context, bounds, []); } + override evaluate(vm: UpdatingVM) { + let { subtreeTag } = this; + + if ( + subtreeTag !== null && + !vm.alwaysRevalidate && + validateTag(subtreeTag, this.subtreeRevision) + ) { + if (LOCAL_DEBUG) { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- @fixme + logStep!('list-item-subtrees', ['skip', this.key]); + } + + // propagate this item's dependencies to any enclosing tracking + // frame, exactly as executing the children would have + consumeTag(subtreeTag); + return; + } + + if (LOCAL_DEBUG) { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- @fixme + logStep!('list-item-subtrees', ['walk', this.key]); + } + + // The frame opened here is closed from the finalizer below, once the + // children have run -- so by then it is not necessarily the innermost + // one. `vm.throw()` unwinds a single frame, and a component's + // `BeginTrackFrameOpcode`/`EndTrackFrameOpcode` pair lives in the same + // ops array as the rest of this item's children, so an `Assert` that + // fires between the two leaves the component's frame open. Closing + // blindly would hand us that frame's partial tag and then skip this + // item forever against it. Recording the depth lets us tell that case + // apart and fall back to "no tag", which only costs a re-render. + let depth = trackFrameDepth(); + + beginTrackFrame(); + vm.try(this.children, this, (didError) => { + let unbalanced = trackFrameDepth() > depth + 1; + let tag: Nullable = null; + + // always balance, even when unwinding; the last frame closed is ours + while (trackFrameDepth() > depth) { + tag = endTrackFrame(); + } + + if (didError || unbalanced || tag === null) return; + + this.subtreeTag = tag; + this.subtreeRevision = valueForTag(tag); + consumeTag(tag); + }); + } + + override handleException() { + // The children are about to be replaced, so the collected tag no longer + // describes them. Belt and braces rather than load-bearing: whatever + // threw did so because a ref this item's tag already covers changed, so + // the tag is invalid regardless and the item would be walked anyway. + // Kept because that reasoning holds for today's `Assert`s, not for any + // future opcode that might unwind on something the tag never saw. + this.subtreeTag = null; + super.handleException(); + } + shouldRemove(): boolean { return !this.retained; } @@ -228,20 +333,30 @@ export class ListBlockOpcode extends BlockOpcode { let iterator = valueForRef(this.iterableRef); if (this.lastIterator !== iterator) { - let { bounds } = this; - let { dom } = vm; + // Deriving a fresh array from tracked state is the idiomatic pattern, + // so the iterator's identity changes on every update even when none + // of the list's keys did. When the new iteration turns out to match + // the existing children one-for-one, the item refs can be updated in + // place -- no marker node, no diff bookkeeping, no children rebuild. + let replay = this.tryFastSync(iterator); + + if (replay !== null) { + let { bounds } = this; + let { dom } = vm; + + let marker = (this.marker = dom.createComment('')); + dom.insertAfter( + bounds.parentElement(), + marker, + expect(bounds.lastNode(), "can't insert after an empty bounds") + ); - let marker = (this.marker = dom.createComment('')); - dom.insertAfter( - bounds.parentElement(), - marker, - expect(bounds.lastNode(), "can't insert after an empty bounds") - ); + this.sync(new PrefixedIterator(replay, iterator)); - this.sync(iterator); + this.parentElement().removeChild(marker); + this.marker = null; + } - this.parentElement().removeChild(marker); - this.marker = null; this.lastIterator = iterator; } @@ -249,6 +364,80 @@ export class ListBlockOpcode extends BlockOpcode { super.evaluate(vm); } + /** + * Walks the new iteration against the existing children, applying it in + * place for as long as it matches. Returns null when everything matched + * in order and in count, which means the update is already complete. + * + * Otherwise the items consumed so far still have to reach the full + * `sync`, which needs the iteration from the beginning -- so the matched + * prefix is rebuilt (from the opcodes, whose refs were just updated) + * along with the item that mismatched. + */ + private tryFastSync(iterator: OpaqueIterator): Nullable { + let { children } = this; + let matched = 0; + + for (;;) { + let item = iterator.next(); + + if (item === null) { + // ran out of items: either an exact match, or the list shrank + return matched === children.length ? null : this.replayPrefix(matched, null); + } + + let opcode = children[matched]; + + if (opcode === undefined || opcode.key !== item.key) { + return this.replayPrefix(matched, item); + } + + updateRef(opcode.memo, item.memo); + updateRef(opcode.value, item.value); + matched++; + } + } + + /** + * The matched prefix was already applied to the item refs, so those items + * can be read back off the opcodes. + * + * The reads are untracked deliberately. This runs inside whatever + * tracking frame happens to be open -- an enclosing `{{#each}}` item's, + * or a component's cache group -- and `valueForRef` consumes. Letting + * these escape would make that frame depend on every item ref in the + * list, so any list mutation would invalidate the enclosing component + * and re-run its update hooks for no reason. + */ + private replayPrefix( + matched: number, + mismatch: Nullable + ): OpaqueIterationItem[] { + let { children } = this; + let prefix: OpaqueIterationItem[] = []; + + beginUntrackFrame(); + + try { + for (let i = 0; i < matched; i++) { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- bounds checked + let opcode = children[i]!; + + prefix.push({ + key: opcode.key, + value: valueForRef(opcode.value), + memo: valueForRef(opcode.memo), + }); + } + } finally { + endUntrackFrame(); + } + + if (mismatch !== null) prefix.push(mismatch); + + return prefix; + } + private sync(iterator: OpaqueIterator) { let { opcodeMap: itemMap, children } = this; @@ -428,7 +617,8 @@ class UpdatingVMFrame { constructor( private ops: UpdatingOpcode[], - private exceptionHandler: Nullable + private exceptionHandler: Nullable, + private finalizer?: (didError: boolean) => void ) {} goto(index: number) { @@ -444,4 +634,39 @@ class UpdatingVMFrame { this.exceptionHandler.handleException(); } } + + finalize(didError: boolean) { + this.finalizer?.(didError); + } +} + +/** + * Replays items the fast path already pulled off an iterator, then drains + * the rest of it, so `sync` can see an iteration from the beginning that + * has in fact been partly consumed. + */ +class PrefixedIterator implements OpaqueIterator { + private index = 0; + + constructor( + private prefix: OpaqueIterationItem[], + private inner: OpaqueIterator + ) {} + + /** + * Only meaningful before the inner iterator has been advanced, which is + * all `sync` needs -- it drives iteration with `next` alone. + */ + isEmpty(): boolean { + return this.index >= this.prefix.length && this.inner.isEmpty(); + } + + next(): Nullable { + if (this.index < this.prefix.length) { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- bounds checked + return this.prefix[this.index++]!; + } + + return this.inner.next(); + } } diff --git a/packages/@glimmer/validator/lib/tracking.ts b/packages/@glimmer/validator/lib/tracking.ts index d94ae456e6b..0df48d5d8b3 100644 --- a/packages/@glimmer/validator/lib/tracking.ts +++ b/packages/@glimmer/validator/lib/tracking.ts @@ -112,6 +112,21 @@ export function isTracking(): boolean { return CURRENT_TRACKER !== null; } +/** + * How many tracking frames are currently open. + * + * A caller that opens a frame and closes it somewhere else -- rather than + * in the same function -- cannot assume the frame it opened is still the + * innermost one by the time it gets to close it, because an unwind in + * between can leave frames open. Recording this before `beginTrackFrame` + * lets such a caller tell "my frame is on top" from "something in between + * leaked", and unwind to a known depth instead of closing a frame that + * belongs to somebody else. + */ +export function trackFrameDepth(): number { + return OPEN_TRACK_FRAMES.length; +} + export function consumeTag(tag: Tag): void { if (CURRENT_TRACKER !== null) { CURRENT_TRACKER.add(tag);