Uh oh!
There was an error while loading. Please reload this page.
fix(dom): reduce transient allocation during serialize (PER-10135) - #2349
fix(dom): reduce transient allocation during serialize (PER-10135)#2349rishigupta1599 wants to merge 3 commits into
Conversation
Percy snapshot serialization of heavy SPAs allocates large transient strings on the hot tail of every capture. On a customer app already near the renderer memory ceiling, that churn adds GC pressure that can contribute to intermittent renderer OOM / browser disconnects. Two safe, output-preserving reductions: - serialize-dom.js: fold the two marker-rewrite `.replace()` passes (custom-element proxies + serialized attributes) into a single regex pass, halving the large-string allocation churn. Output verified byte-identical. - clone-dom.js: in getOuterHTML, when there are no shadow roots to embed as declarative <template shadowrootmode>, return `docElement.outerHTML` directly instead of the getHTML()->textContent=''->outerHTML.replace() reassembly, avoiding a full-size intermediate string + a replace on the common no-shadow-DOM page. Not a complete OOM fix on its own — the full DOM clone dominates the transient peak (see PER-10135 for the memory-constrained reproduction and analysis). Validated as NOT a memory leak: across 100 serializes on one tab the post-GC heap is flat (~0.0005 MB/snapshot) and the live DOM node count is constant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
02e4d8a to
9eb405cCompare
rishigupta1599
left a comment
There was a problem hiding this comment.
Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.
| // Serialized HTML can reach tens of MB and each .replace() copies the whole | ||
| // string, so folding two passes into one halves the large-string churn (GC | ||
| // pressure) per snapshot. Groups: g1 = `<`/`</` (tag); g2/g3 = space + attr. | ||
| const PERCY_MARKER_RE = /(<\/?)data-percy-custom-element-|( )data-percy-serialized-attribute-(\w+?)=/gi; |
There was a problem hiding this comment.
[Low] Merged regex silently widens the tag branch to case-insensitive
The old pair was asymmetric: the custom-element pass was case-sensitive (/(<\/?)data-percy-custom-element-/g) and only the attribute pass had i. Folding them under a single /gi extends i to the tag branch.
I ran a differential fuzz (70k generated inputs plus hand-written edge cases — $&/$1 in attribute values, both markers adjacent, </ closers, empty/hyphen/colon attribute names) against the old two-pass implementation. This is the only divergence found: with lowercase markers the two are byte-identical across 50k cases, but <DATA-PERCY-CUSTOM-ELEMENT-X> is now rewritten to <X> where it previously passed through untouched.
Not reachable from Percy's own output — clone-dom.js:36 builds the tag through document.createElement, which ASCII-lowercases in an HTML document. The exposure is customer page content that literally contains <DATA-PERCY-CUSTOM-ELEMENT-… in a text node, <pre>, <script>, or attribute value, which would now be corrupted in the snapshot.
Suggestion: both markers are always emitted lowercase, so i buys nothing and only widens the blast radius — drop it:
| constPERCY_MARKER_RE=/(<\/?)data-percy-custom-element-|()data-percy-serialized-attribute-(\w+?)=/gi; | |
| constPERCY_MARKER_RE=/(<\/?)data-percy-custom-element-|()data-percy-serialized-attribute-(\w+?)=/g; |
If you'd rather keep the old attribute-branch tolerance exactly, keep i but note it in the comment so the widening is a recorded decision rather than a side effect of merging.
Reviewer: stack:pr-review (orchestrator verification)
| // .replace() reassembly below just reproduces `docElement.outerHTML` while | ||
| // allocating a full-size intermediate string + replace copy. Skip it to cut | ||
| // this step's transient footprint (GC pressure) on heavy pages. | ||
| if (!shadowRootElements || shadowRootElements.length === 0) { |
There was a problem hiding this comment.
[Low] Early return couples shadow-DOM fidelity to an invariant held in another file
The path being skipped did not need this invariant: getHTML({ serializableShadowRoots: true, … }) serializes any shadow root attached with serializable: true — which is exactly how line 126 attaches them — even when the shadowRoots array is empty. Gating on shadowRootElements.length === 0 makes correctness depend on that array being an exact mirror of the clone's shadow roots, and it is populated over in serialize-dom.js:50-66.
I traced it and the invariant holds today: line 126 is the only attachShadow call site in src/, it is always paired with the data-percy-shadow-host stamp under identical disableShadowDOM/forceShadowAsLightDOM conditions (prepare-dom.js:22-33), getShadowRoot() is the same condition clone-dom uses, and serializeElements runs before serializeHTML. So this is a robustness note, not a live bug.
One residual gap worth knowing about: a customer-supplied domTransformation runs at serialize-dom.js:145, after the array is finalized, and can mutate the clone freely — a serializable shadow root attached there would have been picked up by the old path and will now be dropped.
Suggestion: record the coupling where a future reader will see it, e.g. append to the new comment: // Invariant: serializeElements() pushes every clone shadow root into ctx.shadowRootElements (serialize-dom.js:50-66) before serializeHTML runs, so an empty array means the clone has no shadow roots to embed. A test that serializes a shadow-DOM page and asserts the <template shadowrootmode> output survives would pin it against future reordering of serializeDOM.
Reviewer: stack:pr-review (orchestrator verification)
rishigupta1599
commented
Jul 30, 2026
Claude Code PR ReviewPR:#2349 • Head:9eb405c • Reviewers: orchestrator differential verification ( SummaryTwo output-preserving transient-allocation reductions in Verification performedBeyond static review, the two claims of behavioral equivalence were checked directly:
Review Table
Findings
Note (positive): the early return also sidesteps a latent bug in the path it skips — Note on the perf claim: the change reduces churn (one full-size string allocation instead of three) but not the peak — the DOM clone is still live while
Verdict: PASS — both changes are equivalence-verified (fuzz + invariant trace + green CI), scoped tightly, and well commented. The four findings are all Low: drop the now-unnecessary |
rishigupta1599
left a comment
There was a problem hiding this comment.
Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.
| // .replace() reassembly below just reproduces `docElement.outerHTML` while | ||
| // allocating a full-size intermediate string + replace copy. Skip it to cut | ||
| // this step's transient footprint (GC pressure) on heavy pages. | ||
| if (!shadowRootElements || shadowRootElements.length === 0) { |
There was a problem hiding this comment.
[High] Wrong predicate — silently drops shadow DOM attached during domTransformation
Upgraded from the Low note posted earlier on this line: this was verified as reachable, with a reproduction, rather than hypothetical.
getHTML({ serializableShadowRoots: true, shadowRoots: [] })does serialize a shadow root marked serializable: true even when it is absent from the array (verified in Chrome). The array only matters for non-serializable roots — such as the constructor-created root reused at lines 122-124, which is why serialize-dom.test.js:332 expects <template shadowrootmode="open"> with no shadowrootserializable. So shadowRootElements.length === 0 is not equivalent to "the clone has no serializable shadow roots".
The reachable path: a domTransformation callback runs at serialize-dom.js:145, i.e. afterserializeElements has finished populating the array, and can attach a serializable shadow root to the clone. Repro:
serializeDOM({domTransformation: (docEl)=>{constsr=docEl.querySelector('#content').attachShadow({mode: 'open',serializable: true});sr.innerHTML='<p>PROBETRANSFORM</p>';}})master → html contains PROBETRANSFORM; patched → <div id="content"></div>, content gone, no warning emitted. domTransformation / dom_transformation is a documented public SDK option.
Ruled out as divergent (no issue there): live-DOM shadow roots — markElement (prepare-dom.js:23-33) and clone-time attachShadow (lines 116-133) are gated on the identical condition; nested and CDP-captured closed roots share one array; iframes get their own ctx via serializeFrames. domTransformation is the one gap.
Suggestion: gate on both conditions — thread a flag from serialize-dom.js:29 (cloneMayHaveUncountedShadowRoots: !!domTransformation) and test if (!cloneMayHaveUncountedShadowRoots && !shadowRootElements?.length). Land the repro as a regression test. (Nit while here: the !shadowRootElements || arm is unreachable — the sole caller always passes an array — so !shadowRootElements?.length suffices.)
Reviewer: stack-code-reviewer
| // allocating a full-size intermediate string + replace copy. Skip it to cut | ||
| // this step's transient footprint (GC pressure) on heavy pages. | ||
| if (!shadowRootElements || shadowRootElements.length === 0) { | ||
| return docElement.outerHTML; |
There was a problem hiding this comment.
[Medium] Skipping textContent = '' works against the PR's own memory goal
The reassembly path clears the clone at line 177 before returning. That was not incidental: it released the entire cloned node tree before the caller built doctype(ctx.dom) + html (serialize-dom.js:35) and — with stringifyResponse — a second full-size JSON copy of the html (serialize-dom.js:183).
With this early return the clone stays fully reachable across all of that, so peak retention rises by roughly the size of the clone. The PR's own investigation identifies that clone as the dominant contributor (~315 MB for the 45k-node repro, against a ~30 MB html string), so on a tight memory boundary this plausibly costs more than the one saved string copy gains.
Nothing depends on the emptying: serializeHTML is the last consumer of ctx.clone, and cleanupInteractiveStateMarkers only walks ctx._liveMutations.
Suggestion: keep the release in the fast path:
| returndocElement.outerHTML; | |
| consthtml=docElement.outerHTML; | |
| docElement.textContent=''; | |
| returnhtml; |
The pre-existing forceShadowAsLightDOM and old-Firefox early returns have the same omission and would benefit equally.
Reviewer: stack-code-reviewer
rishigupta1599
commented
Jul 30, 2026
Claude Code PR Review (revised)PR:#2349 • Head:9eb405c • Reviewers: stack-code-reviewer, orchestrator differential verification
SummaryTwo intended-output-preserving transient-allocation reductions in Verification performed
Review Table
Findings
Settled, no action needed: the callback's Out of scope: an automated scan flagged Verdict: FAIL — F1 is silent snapshot data loss on a documented public option, with a reproduction, and F2 means the change likely worsens peak retention on the very OOM path it targets. Both are small, well-understood fixes (thread a flag into the guard; keep the |
…elease Addresses review findings on the getOuterHTML fast path added in this PR. An empty `shadowRootElements` is not by itself proof that the clone has no shadow roots to serialize: `getHTML`'s `serializableShadowRoots: true` also picks up roots attached with `serializable: true` — which is how `cloneNodeAndShadow` attaches them — even when absent from the explicit `shadowRoots` array. The array is authoritative only because `serializeElements` populates it before `serializeHTML` runs, and a caller-supplied `domTransformation` breaks that: it runs afterwards and can attach a serializable root to the clone. Those snapshots silently lost their shadow content. The fast path is now gated on both conditions. The fast path also skipped the `textContent = ''` that the reassembly path performs, keeping the entire clone reachable while the caller built the doctype concatenation and, with `stringifyResponse`, a full JSON copy on top of it. On a heavy page the retained tree dwarfs the single string copy the fast path saves, working against this PR's own objective; the release is restored. Also corrects the marker-regex comment (a non-matching global `.replace()` does not copy the string, so the win is narrower than claimed) and documents the `i` flag now covering the previously case-sensitive tag branch. Tests: adds a `getOuterHTML` unit suite (byte-equality against the reassembled form, clone release, shadow roots absent from the array) and a `domTransformation` + shadow-root regression case. All three fail without the corresponding fix. Adds direct marker un-mangling assertions, which nothing in the suite previously covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rkers The previous fixture used an undefined custom element with no attributeChangedCallback, so cloneElementWithoutLifecycle took the plain cloneNode() branch and emitted no markers at all — every assertion passed on input that never contained one. Verified vacuous by neutering the tag branch of PERCY_MARKER_RE: the test still passed. Uses a proxied custom element carrying a `src` instead, which puts the tag marker and the serialized-attribute marker in the same tag — the case the single-pass replacer's branch discrimination has to get right. Re-verified under the same neutering: it now fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rishigupta1599
commented
Jul 30, 2026
Claude Code PR Review (re-review after fixes)PR:#2349 • Head:6ddd033 • Reviewers: stack-code-reviewer, orchestrator differential verification
SummaryTransient-allocation reductions in What changed since 9eb405c
Verification performedF1 closure (reviewer, in headless Chrome at F2 safety (reviewer). Tests are not tautological. Reverting the guard makes 3 of the new tests fail (clone release; shadow-roots-absent-from-array; Coverage. CI on Review Table
FindingsNo blocking findings remain. Open items (deliberate, for the PER-10135 follow-up)
Verdict: PASS — both blocking findings are fixed, the fixes are independently verified rather than asserted, and the regression tests were each confirmed to fail without their fix. The remaining items are pre-existing or deliberate scope decisions, recorded above for the follow-up. |
This PR is stale because it has been open for more than 14 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
Context — PER-10135
Customer (RS Components / LCNC, Selenium SDK) sees intermittent renderer OOM / browser disconnects during
PercyDOM.serializeon long (~50 min) sessions, both Chrome and Firefox.Root cause (investigated + reproduced)
Not a memory leak. A single
serializeproduces a large transient allocation burst (full DOM clone + full HTML string + string copies +JSON.stringify/bridge marshalling). On a customer SPA already near the renderer/VM memory ceiling (telemetry p50 heap ~1885 MB) in a constrained Automate VM, that one-shot burst intermittently tips the renderer over.docker --memory): identical 45k-node/315 MB page survives at 2 GB (heap sawtooths & recovers) but crashes on the first serialize at 1.1 GB. Roomy hosts can't repro it (--max-old-space-sizedoesn't bound a renderer) — which is why earlier attempts failed.What this PR changes (safe, output-preserving)
Two reductions to Percy's per-snapshot transient footprint:
serialize-dom.js— fold the two marker-rewrite.replace()passes (custom-element proxies + serialized attributes) into a single regex pass, halving large-string allocation churn on the hot tail of every capture. Output verified byte-identical against representative inputs.clone-dom.js— ingetOuterHTML, when there are no shadow roots to embed, returndocElement.outerHTMLdirectly instead of thegetHTML() → textContent='' → outerHTML.replace()reassembly, avoiding a full-size intermediate string + a replace on the common no-shadow-DOM page.Honest scope
This is a mitigation, not a complete OOM fix — the reproduction shows the full DOM clone dominates the transient peak, so these string-copy savings don't move a tight boundary on their own. Larger follow-ups (streamed/chunked snapshot transfer to avoid the in-renderer
JSON.stringify+ bridge double-marshal; capping huge inline content; customer guidance) are tracked in PER-10135.Testing
@percy/domkarma test suite passes.🤖 Generated with Claude Code