[PERF] Give Glimmer's own destroyables a meta slot - #21568
[PERF] Give Glimmer's own destroyables a meta slot#21568NullVoxPopuli-ai-agent wants to merge 3 commits into
Conversation
`getDestroyableMeta` is the hottest Glimmer function in a CPU profile of `smoke-tests/benchmark-app`, at 1.7% self time. Every association costs a `WeakMap.get`, and the first use of an object as a `WeakMap` key forces an identity hash onto it. Rendering a list associates one per item. Classes Glimmer constructs declare a slot for their meta and skip the map: `BlockOpcode` and its subclasses, and the VM's root destroyable. Meta is only written to the slot when the slot already exists, so anything arriving from outside falls through to the `WeakMap` and is never touched. That matters because destroyables are public through `@ember/destroyable` and can be any object a user hands in. Writing meta onto them would break frozen and sealed instances, and a spread copy would share one destroy state between two instances. Worth 15% of VM time on that workload, measured in Node against SimpleDOM, 15 interleaved pairs: | phase | main | branch | delta | | ----------------- | ----: | -----: | -----: | | update every 10th | 12.30 | 5.86 | -50.6% | | clear | 8.96 | 6.36 | -31.9% | | select row | 2.72 | 2.05 | -22.6% | | append 1000 | 37.57 | 32.50 | -11.2% | | total | 88.54 | 74.28 | -15.3% | Tests: 9449 tests, 9432 pass, 17 skip, 0 fail. Identical to main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
These three read `DESTROYABLE_META` directly, so a slotted destroyable missed
its own meta and answered `false` to all three, including while it was
destroying.
Nothing calls them on a `BlockOpcode` today, so the suite did not catch it.
`invokeHelper` does call `isDestroying` and `isDestroyed` on its cache, which
is the next thing worth slotting, so this had to be right first.
slotted object, before
during destroy: isDestroying = false
after destroy: isDestroyed = false
slotted object, after
during destroy: isDestroying = true
after destroy: isDestroyed = true
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The slot check costs an `in` test for every object that does not have one, and without this the check has nothing to pay for it. rere-benchmark caught the result: `1k items, 1 update on 5% (random, async)` was 3.9%, 8.3% and 13.4% slower than main across three rounds. An empty slot is proof the object is one of ours and has no meta, so the map lookup can be skipped. `VM_HELPER_OP` asks `_hasDestroyableChildren` on every helper invocation and almost always gets `false`. Neither `pnpm bench` nor a Node microbenchmark showed the regression. Both are dominated by mass insertion and teardown; this one is a fine-grained update path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Stacked on emberjs#21568. Three more classes Glimmer constructs itself get the slot: `ReferenceImpl`, `RenderResultImpl` and `AppendingBlockImpl`. Component and modifier destroyables are deliberately left alone. `manager.getDestroyable(state)` hands back the user's own instance, and writing to those is what made emberjs#21565 unmergeable. The symbol moves to `@glimmer/util` so that `@glimmer/reference` can declare the slot. It does not depend on `@glimmer/destroyable`, and moving a symbol down is lighter than adding that edge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
rere-benchmark found a regression in this PR as originally filed, and it is now fixed on the branch. The slot check costs an
Slower in all three rounds. Neither The short-circuit now lives here, so this PR stands on its own. Re-measuring on rere now and I will post the numbers. |
|
Re-measured on rere-benchmark after moving the short-circuit into this PR. Three rounds, The regression is gone:
Consistent across all three rounds now:
No bench is consistently slower. On the mass insertion and teardown side, One caution for anyone reading per-bench numbers here. |
Replaces #21565, which did the same thing in a way that was breaking.
getDestroyableMetais the hottest Glimmer function in a CPU profile ofsmoke-tests/benchmark-app, at 1.7% self time. Every association costs aWeakMap.get, and the first use of an object as aWeakMapkey forces an identity hash onto it. Rendering a list associates one per item, so a 10,000 row list pays about 40,000 lookups before anything is destroyed.Classes Glimmer constructs declare a slot for their meta and skip the map. Everything else keeps the
WeakMap, untouched.Numbers
VM work, measured in Node against SimpleDOM so DOM cost does not mask it. 15 interleaved pairs, medians in ms:
Browser,
pnpm benchat fidelity 20 and 8x CPU throttle:No phase regresses. For scale, running the same harness from a
mainworktree against itself reporteddurationas no difference across -1368ms to +739ms, so a total whose whole interval sits below zero is a real result. That A/A run also produced two falsely significant phase results at about 7%, so read the total first.#21565 measured -3.66% on the same harness. This keeps about two thirds of that, and gives up none of
@ember/destroyable's guarantees.How it avoids touching user objects
Destroyables are public through
@ember/destroyable, and a destroyable can be any object a user hands in. The docs' own example isassociateDestroyableChild(this, {}).So the rule is: read the slot always, write it only when the slot already exists.
Only
BlockOpcodeand its subclasses (TryOpcode,ListItemOpcode,ListBlockOpcode) and the VM's root destroyable declare the slot. Those are what the list benchmark associates per item.Why #21565 could not ship
It wrote the meta onto the destroyable unconditionally. Probing the built branch found four problems, and the second is the one that killed it:
registerDestructor(Object.freeze({}), fn)settrap firesThe second row means two distinct instances could report a single destroy state, since a plain assignment creates an enumerable own symbol and spread copies it:
Instances need their own destroy state. Under this PR a user instance is never written to, so that cannot happen.
Also in here
Destroyable tracking enumerates meta to find leaks, and slotted objects bypass the
WeakMap. WhileenableDestroyableTracking()is on, the slot path also records into the tracking map, soassertDestroyablesDestroyed()still sees them. All 99 destroyable tests pass.Testing
9449 tests, 9432 pass, 17 skip, 0 fail. Identical to
mainon the same machine.tsc,eslintandprettierare clean.Next
Component managers, modifiers and helper caches are also framework-owned and still take the
WeakMappath. Widening the slot to them is a follow-up rather than more surface here.