[PERF] Store destroyable meta on the destroyable - #21565
[PERF] Store destroyable meta on the destroyable#21565NullVoxPopuli-ai-agent wants to merge 1 commit into
Conversation
BREAKING. Opening this for the numbers and the discussion, not to merge as is.
`getDestroyableMeta` is the hottest Glimmer function in a CPU profile of
`smoke-tests/benchmark-app`, at 1.7% self time. Every associated destroyable
costs a `WeakMap.get`, and the first use of an object as a `WeakMap` key also
forces an identity hash onto it. Rendering a list associates one per item.
Moving meta onto the destroyable under a symbol is worth 23% of VM time on that
workload. Measured in Node against SimpleDOM so DOM cost does not mask it,
15 interleaved pairs, medians in ms:
| phase | main | branch | delta |
| ----------------- | -----: | -----: | -----: |
| create | 32.60 | 25.11 | -24.4% |
| update every 10th | 11.14 | 8.87 | -24.4% |
| select row | 5.90 | 4.73 | -17.7% |
| swap rows | 5.87 | 5.21 | -13.5% |
| remove row | 5.40 | 4.79 | -4.1% |
| append 1000 | 53.08 | 39.48 | -25.6% |
| clear | 13.93 | 10.16 | -26.1% |
| total | 132.73 | 103.40 | -23.1% |
Why it is breaking:
main: registerDestructor(Object.freeze({}), fn) -> OK
branch: registerDestructor(Object.freeze({}), fn) -> TypeError
`registerDestructor`, `destroy` and `associateDestroyableChild` are public
through `@ember/destroyable`, and they accept frozen and sealed objects today.
A symbol property is also visible to `Reflect.ownKeys` and
`Object.getOwnPropertySymbols`.
Destroyable tracking needs its own registry, because meta is no longer
enumerable. It is a `Set` populated only between `enableDestroyableTracking()`
and `assertDestroyablesDestroyed()`.
Tests: 9449 tests, 9432 pass, 17 skip, 0 fail. Identical to main.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Benchmarked against Total: -3.66% [-6.26% to -1.41%]. No phase regresses.
For scale, I ran the same harness from a Every clear phase improves. That is where Worth comparing against #21564, which keeps the So the ordering is clear: this is the destroyable win, and it is the largest single change measured in this batch. It still throws on frozen and sealed destroyables, which is the open question at the top of the description. |
|
Superseded by #21568. Closing. The approach there keeps user objects untouched: Glimmer's own classes declare a meta slot and skip the That removes all four problems this PR had, the important one being that a spread copy shared one destroy state between two instances. Cost of doing it safely, same harness both times:
About two thirds of the win, and it is mergeable. |
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>
Breaking. Filed for the numbers and the discussion, not to merge as is. Split out of #21564 at @NullVoxPopuli's request.
getDestroyableMetais the hottest Glimmer function in a CPU profile ofsmoke-tests/benchmark-app, at 1.7% self time. Every associated destroyable 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.Storing meta on the destroyable under a symbol instead is worth 23% of VM time.
Numbers
Measured in Node against SimpleDOM, so DOM cost does not mask the VM. 15 interleaved pairs, medians in ms:
I have not measured this one alone through
pnpm bench. On that harness, browser DOM work dominates and an A/A run reports false positives of about 7% on individual phases, so a single phase number there would not mean much.Why it is breaking
registerDestructor,destroyandassociateDestroyableChildare public through@ember/destroyable, and they accept frozen and sealed objects today. A symbol property is also visible toReflect.ownKeysandObject.getOwnPropertySymbols.Nothing in the framework freezes a destroyable and no test covers it, so the suite passes either way. That is the decision this PR exists to raise.
If the answer is that Ember cannot break it, one option is a symbol with a
WeakMapfallback for non-extensible objects. That keeps the fast path and never throws, at the cost of anObject.isExtensiblecheck when meta is first created. I have not measured that variant.Also in here
Destroyable tracking needs its own registry, because meta is no longer enumerable. It is a
Setpopulated only betweenenableDestroyableTracking()andassertDestroyablesDestroyed().Testing
9449 tests, 9432 pass, 17 skip, 0 fail. Identical to
mainon the same machine.tsc,eslintandprettierare clean.Conflicts textually with #21564, which also touches
destroy.