test(xterm): pre-#617 kolu + observer-nullify on latest xterm master#668
Draft
test(xterm): pre-#617 kolu + observer-nullify on latest xterm master#668
Conversation
The original #617 repro — 30× Focus↔Canvas sidebar mode toggle with 7 terminals — no longer exists in current kolu master because #622 made the workspace mode-less. To re-run that exact repro with the observer-nullify variant instead of WeakRef, this branch rewinds kolu to the parent of #617's merge commit (where the sidebar toggle still exists) and bumps the xterm override to latest upstream master stacked with just the nullify patch: juspay/xterm.js#test/kolu-nullify-on-latest-master-built = xtermjs/xterm.js@32553b41 + observer-nullify The branch does NOT carry the WeakRef fix that landed in #5821 / #617. The IntersectionObserver callback uses a strong `this` capture; the observer is stored on `this._observer` and nulled on dispose. Test plan (on your real Chrome, not chrome-devtools MCP): 1. Check out this branch, `just dev` 2. Restore 7 terminals 3. Toggle 30× between Focus and Canvas sidebar mode (Ctrl+Shift+C or the sidebar icon) 4. Chrome Task Manager: note Memory Footprint Δ Arm cross-reference: - Arm A (unpatched baseline): git checkout 1b18af1^ - Arm B (WeakRef, #617 merged): git checkout 1b18af1 - Arm C (observer-nullify): this branch Expected per #617: Arm A +367 MB / 30 toggles, Arm B ~0 MB. The open question Arm C answers: does observer-nullify defuse the retention as effectively as WeakRef? Not for merge. Data collection only. Refs xtermjs/xterm.js#5821, #617, #652, #653
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
1 participant
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.
Why this branch exists
The original #617 repro — 30× Focus↔Canvas sidebar mode toggle with 7 terminals, producing +367 MB / 30 toggles on pureintent — isn't reproducible on kolu master anymore because #622 made the workspace mode-less (no sidebar toggle, unified infinite canvas).
So to answer jerch's question on xtermjs/xterm.js#5821 — does the observer-nullify variant defuse the retention as effectively as WeakRef? — I need the exact same kolu state that #617 was measured against, only with the observer-nullify fork spliced in instead.
This branch does that:
1b18af1^(parent of fix(xterm): patch IntersectionObserver retention (-367 MB/30 mode-toggles) #617's merge commit) → sidebar toggle still present@xterm/xtermoverride →juspay/xterm.js#test/kolu-nullify-on-latest-master-built, which is latestxtermjs/xterm.js@master(commit32553b41) + the nullify patch (lib/xterm.mjsseeded from coordinated@xterm/xterm@6.1.0-beta.200+@xterm/addon-webgl@0.20.0-beta.199, both published from the same SHA).No
WeakRefanywhere in the IntersectionObserver callback lineage.Three-arm result
All three arms share the same kolu UI (pre-#622, sidebar toggle alive). The MCP Chrome measurement used fresh
just devruns, clean.kolu-devwhere needed, 7 terminals created/restored through the UI, and 30 Focus↔Canvas round trips via the visible header toggle.1b18af1^juspay/xterm.js#fix/dispose-leaks-built(pre-#5821)Uint32Array +339,JSArrayBufferData +3391b18af1juspay/xterm.js#fix/kolu-xterm-fixes-built(dispose-leaks + WeakRef)Uint32Array +0,JSArrayBufferData +0juspay/xterm.js#test/kolu-nullify-on-latest-master-built(upstream master + nullify)Uint32Array +0,JSArrayBufferData +0Heap snapshot self-size deltas were noisy because DevTools itself retained other frontend objects during the run, but the xterm buffer signature is the important signal here:
JSArrayBufferDatabytes ΔUint32Arraycount ΔCaveat: this MCP run reproduced the leak signature on A, but not the original +367 MB magnitude because the clean UI-created/restored terminals only had ~118 rows of scrollback rather than the large restored buffers from the pureintent measurement.
Answering jerch
Observer-nullify defuses the same xterm buffer retention leak as the merged WeakRef fix. In the same Chrome DevTools MCP methodology, the unpatched baseline retained new xterm buffer arrays after 30 toggles, while both WeakRef and observer-nullify retained zero additional xterm buffer arrays.
That means the upstream PR can be simplified to the observer-nullify variant without relying on
WeakRefin the IntersectionObserver callback lineage.Rollback
Not for merge. Close after data is collected. The real outcome lands as a change on #5821.
Refs xtermjs/xterm.js#5821, #617, #620, #652, #653