Perf: replace splice with swap-and-pop in destroyable remove() (runtime)#23
Merged
Perf: replace splice with swap-and-pop in destroyable remove() (runtime)#23
Conversation
`remove()` in `@glimmer/destroyable/index.ts:58` is invoked every time
a destroyable is unassociated from its parent (happens during the
destroy phase, from `removeChildFromParent`). Its previous form:
let index = collection.indexOf(item);
collection.splice(index, 1);
does an O(n) shift of all elements after `index`. For a parent with
thousands of children being cleared, the total cost is O(n²).
Swap-with-last-then-pop is O(1) for the splice piece (the indexOf
lookup above it is unchanged):
let index = collection.indexOf(item);
let lastIndex = collection.length - 1;
if (index !== lastIndex) {
collection[index] = collection[lastIndex] as T;
}
collection.pop();
Order is not observable. The only consumer of the collection is
`iterate()` (uses `.forEach` for destructors/parents/children); none
of the callers — destructor firing, parent/child propagation —
assume sibling order. Parent-side removal is also batched via
`scheduleDestroyed`, so a child is never removed from the collection
while the parent is iterating it.
Measured with tracerbench (20-fidelity compare vs origin/main):
| phase | Δ ms | Δ % |
|----------------------|------------:|---------:|
| clearManyItems1End | **-43 ms** | **-21.3 %** |
| clearManyItems2End | **-40 ms** | **-39.5 %** |
| render1000Items1End | -2 ms | -2.9 % |
| (all other 17 phases)| no diff | |
90% CIs for the two clear-5000 phases: [-46, -40] ms and [-46, -36] ms
respectively — well outside the regression/noise threshold tracerbench
uses. No regressions on any measured phase.
📊 Package size report 0%↑
🤖 This report was automatically generated by pkg-size-action |
3 tasks
Skips the unused return-value read from pop(). Semantically identical (both truncate the array by one; the packed-array fast path is preserved in either form). Also byte-identical to the same change in NVP's upstream emberjs#21221, simplifying side-by-side review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
remove()inpackages/@glimmer/destroyable/index.tsis invoked every time a destroyable is unassociated from its parent (during the destroy phase, viaremoveChildFromParent). The old form did an O(n) element shift on every removal:For a parent with N children being cleared one-by-one, the total cost across the clear is O(N²) in the splice piece alone (the
indexOfis also O(n) but thespliceshift of the trailing elements is the dominant memory-copy work).New form swaps the removed item with the last element and truncates — O(1) for the splice piece;
indexOfis unchanged:Prior art
The same change has already been discovered (alongside other runtime tweaks) in NVP's open upstream PR emberjs/ember.js#21221 "Improve render perf" —
This PR is intentionally narrow: just the destroyable change, with a tracerbench 20-fidelity compare attributing the win. I have not reproduced any more concrete wins from the other fixes from emberjs#21221 yet.
Why this is semantics-identical
Collection order is not observable to anything outside the destroyable module. The only consumers are:
iterate(collection, fn)(lines 50–56) — usesArray.prototype.forEach, which traverses whatever the current in-memory order is. None of its callers (destroy()at line 163 forchildren/eagerDestructors/destructors/parents) assume a particular order among siblings. Destructors are peers; the destroyable contract says children are destroyed before their parents, but says nothing about sibling ordering.scheduleDestroyed(line 180), so a child is never spliced out of a parent collection while the parent is actively iterating it — no iteration-during-mutation corner cases.The only observable behaviour change between
spliceand swap-and-pop is the order of the remaining elements after a removal, which — per the above — is not visible to any consumer.Before / after
TracerBench 20-fidelity compare (Krausest-style benchmark-app,
origin/mainvs this branch), M1 Max / Node 24.14 / Chrome Canary:The wins are concentrated in the clear-5000-items phases — exactly where the old
spliceshift was quadratic. No regression in any of the 20 measured phases. Overall duration is −108 ms / −4.6 % median (CI [−144, −47] ms).The two clear-5000 phases go from ~203 ms → ~159 ms and ~106 ms → ~63 ms respectively; together they account for nearly the entire duration delta.
Reproducing the benchmark
Commands are meant to be run from the ember.js repo root. Requires pnpm and Chrome Canary installed (TracerBench launches Canary by default).
Outputs land in
tracerbench-testing/experiment/tracerbench-results/:control/experimentgroups, each withsamples[*].phases[*].durationin microseconds)To extract the absolute phase medians in the format used above:
Runtime: ~10–15 min including both tarball builds and 40 Chrome sessions.
Test plan
pnpm testgreenpnpm lintgreenremovethroughremoveChildFromParentthoroughly (association / unassociation / destroy flows).