Skip to content

Pool per-frame and per-render allocations in the update path - #21543

Closed
NullVoxPopuli-ai-agent wants to merge 2 commits into
emberjs:mainfrom
NullVoxPopuli-ai-agent:extract-tracker-pool
Closed

Pool per-frame and per-render allocations in the update path#21543
NullVoxPopuli-ai-agent wants to merge 2 commits into
emberjs:mainfrom
NullVoxPopuli-ai-agent:extract-tracker-pool

Conversation

@NullVoxPopuli-ai-agent

Copy link
Copy Markdown
Contributor

Extracted from the #21520 performance spike: the two allocation pools that are pure implementation details — no observable behavior change, no API surface, full test suite green locally (9443 tests, 0 failures).

  1. Tracking-frame Trackers: frames are strictly LIFO, so finished Trackers reset into a small free list instead of allocating a Tracker + Set on every beginTrackFrame. Instances abandoned on the error path (resetTracking) are never repooled.
  2. Per-render transaction + updating VM: every render allocates a fresh TransactionImpl (with four arrays) and every rerender a fresh UpdatingVM. Both now reuse a single pooled instance under discard-on-exception discipline — repooled only after a fully clean commit/execute, so any throw leaves the pool empty and error paths allocate fresh, keeping recovery behavior byte-for-byte unchanged.

Honest framing on impact: on the spike's benchmark suite these measured neutral (allocation was no longer the bottleneck there after its other levers); on main's hotter allocation profile they reduce steady-state garbage in every render. They're extracted because they're free, safe, and shrink the diff of future update-path work.

Related: #21520 (spike), #21493 (RFC 957 interface).

🤖 Generated with Claude Code

NullVoxPopuli-ai-agent and others added 2 commits August 6, 2026 14:43
Tracking frames are strictly LIFO, so finished Trackers can be reset
into a small free list instead of allocating a Tracker and its Set on
every beginTrackFrame. Instances abandoned on the error path
(resetTracking) are never repooled; depth beyond the cap allocates as
before.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every render allocates a fresh TransactionImpl (with four arrays) and
every rerender a fresh UpdatingVM. Both now reuse a single pooled
instance, repooled only after a fully clean commit/execute -- any
throw leaves the pool empty, so error paths allocate fresh and
recovery behavior is byte-for-byte unchanged. The updating VM's frame
stack is necessarily empty after a clean execute, so re-arming is
field reassignment only.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@NullVoxPopuli

NullVoxPopuli commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@NullVoxPopuli-ai-agent is this actually a good use of resources?

it kinda looks like this PR does nothing, but with extra steps.

in the original code, we create a vm, and then it would be garbage collected when the rerender function finishes, yea?

in the new code,

  • releaseUpdatingVM, doesn't release, it assigns a module-state variable to a value
  • aquireUpdatingVM never sets pooledUpdatingVM to a non-null value.
  • is "releaseUpdatingVM" being used to share a vm across "simultaneous" calls to rerender? (also, does that even happen?) and wouldn't that be brittle if you have two different apps booted? (that each ned their own VM)

here are the pnpm bench results from this PR (against main):

run 1

duration phase no difference [-45ms to 19ms]
renderEnd phase no difference [0ms to 1ms]
render1000Items1End phase no difference [-2ms to 0ms]
clearItems1End phase no difference [-1ms to 1ms]
render1000Items2End phase no difference [-1ms to 3ms]
clearItems2End phase no difference [0ms to 0ms]
render10000Items1End phase no difference [-5ms to 9ms]
clearManyItems1End phase no difference [0ms to 3ms]
render10000Items2End phase no difference [-26ms to 12ms]
clearManyItems2End phase estimated improvement -3ms [-4ms to -2ms] OR -7.71% [-10.53% to -4.53%]
render1000Items3End phase no difference [-2ms to 1ms]
append1000Items1End phase no difference [-3ms to 3ms]
append1000Items2End phase no difference [-1ms to 8ms]
updateEvery10thItem1End phase no difference [-2ms to 1ms]
updateEvery10thItem2End phase no difference [-2ms to 2ms]
selectFirstRow1End phase estimated improvement -2ms [-3ms to -1ms] OR -19.64% [-25.72% to -10.81%]
selectSecondRow1End phase estimated regression +2ms [1ms to 3ms] OR +28.09% [12.89% to 37.67%]
removeFirstRow1End phase no difference [-2ms to 1ms]
removeSecondRow1End phase no difference [-2ms to 1ms]
swapRows1End phase no difference [-2ms to 1ms]
swapRows2End phase no difference [-2ms to 0ms]
clearItems4End phase no difference [-1ms to 0ms]
paint phase no difference [-2ms to 1ms]
run 2
duration phase no difference [-37ms to 14ms]
renderEnd phase no difference [0ms to 1ms]
render1000Items1End phase no difference [-3ms to 1ms]
clearItems1End phase no difference [-2ms to 0ms]
render1000Items2End phase estimated regression +1ms [0ms to 3ms] OR +3.43% [0.81% to 7.59%]
clearItems2End phase no difference [0ms to 1ms]
render10000Items1End phase no difference [0ms to 8ms]
clearManyItems1End phase no difference [0ms to 2ms]
render10000Items2End phase no difference [-10ms to 8ms]
clearManyItems2End phase estimated improvement -3ms [-4ms to -3ms] OR -8.49% [-11.27% to -6.37%]
render1000Items3End phase no difference [-1ms to 1ms]
append1000Items1End phase no difference [-3ms to 3ms]
append1000Items2End phase no difference [-1ms to 2ms]
updateEvery10thItem1End phase no difference [-3ms to 1ms]
updateEvery10thItem2End phase no difference [-3ms to 0ms]
selectFirstRow1End phase estimated improvement -3ms [-3ms to -2ms] OR -24.08% [-31.83% to -19.62%]
selectSecondRow1End phase estimated regression +2ms [1ms to 3ms] OR +30.77% [17.04% to 41.7%]
removeFirstRow1End phase no difference [-2ms to 0ms]
removeSecondRow1End phase no difference [-2ms to 0ms]
swapRows1End phase no difference [-2ms to 1ms]
swapRows2End phase no difference [-1ms to 1ms]
clearItems4End phase estimated improvement -1ms [-2ms to 0ms] OR -6.04% [-11.49% to -1.67%]
paint phase estimated improvement -2ms [-4ms to 0ms] OR -33.81% [-61.65% to -8.34%]

rere-benchmark results:
image

not sure which view is better

image

so, it looks like in most cases, this PR is worse for us.

What do your own rere-benchmark results say about this one PR?

@NullVoxPopuli-ai-agent

Copy link
Copy Markdown
Contributor Author

Ran the isolated same-batch comparison this PR should have shipped with (today's main vs main+this branch, 8x throttle, 5 runs interleaved, mutation-anchored): my data agrees with yours — this PR is neutral-to-worse, with one real regression.

bench main this PR
DB Monitor 7.2 fps 7.3 fps (neutral)
1k items, 1 update each (seq, async) 1492–1736ms 1865–3308ms — real, up to ~2x worse (four of five runs above main's entire range)
Incrementing 4291ms 4436ms (~3% worse)
most others mixed ±10%, overlapping spreads

To be accountable about it: the spike's own measurements had already scored these pools neutral in their original context; the "on main's hotter allocation profile they reduce steady-state garbage" line in this PR's description was projection, not measurement, and the isolated run now falsifies it — pooling doesn't just fail to help here, the reuse plausibly costs more than young-gen allocation (retained Set/array capacity, an added polymorphic site in beginTrackFrame) on walk-heavy paths.

A lever with no measured win and one measured regression shouldn't spend anyone's review time. Closing. The lesson feeds the other extraction PRs' standard: same-batch isolated numbers on main, not spike-context claims.

🤖 Generated with Claude Code

NullVoxPopuli-ai-agent added a commit to NullVoxPopuli-ai-agent/ember.js that referenced this pull request Aug 6, 2026
Isolated main-vs-PR measurement (emberjs#21543) showed the pooling set
neutral at best and genuinely regressive on walk-heavy paths -- reuse
costs more than young-generation allocation there. The spike's own
batches had already scored it neutral. A lever with no win has no
place in the showcase.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants