Skip to content

fix(dom): reduce transient allocation during serialize (PER-10135) #2349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rishigupta1599 wants to merge 3 commits into
base: master
Choose a base branch
from
+179 −6
Open

fix(dom): reduce transient allocation during serialize (PER-10135) #2349

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9eb405c
fix(dom): reduce transient allocation during serialize (PER-10135)
rishigupta1599 Jul 22, 2026
fc7b9a0
fix(dom): gate serialize fast path on domTransformation, keep clone r…
rishigupta1599 Jul 30, 2026
6ddd033
test(dom): make the marker un-mangling test actually exercise both ma…
rishigupta1599 Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/dom/src/clone-dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,13 @@ export function getOuterHTML(docElement, { shadowRootElements, forceShadowAsLigh
if (forceShadowAsLightDOM) {
return docElement.outerHTML;
}
// With no shadow roots to embed, the getHTML()+textContent=''+outerHTML
// .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) {

@rishigupta1599 rishigupta1599 Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 rishigupta1599 Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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. after serializeElements has finished populating the array, and can attach a serializable shadow root to the clone. Repro:

serializeDOM({ domTransformation: (docEl) => {
  const sr = 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

return docElement.outerHTML;

@rishigupta1599 rishigupta1599 Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

Suggested change
return docElement.outerHTML;
const html = docElement.outerHTML;
docElement.textContent = '';
return html;

The pre-existing forceShadowAsLightDOM and old-Firefox early returns have the same omission and would benefit equally.

Reviewer: stack-code-reviewer

}
/* istanbul ignore else if: Only triggered in chrome <= 128 and tests runs on latest */
if (docElement.getHTML) {
// All major browsers in latest versions supports getHTML API to get serialized DOM
Expand Down
14 changes: 10 additions & 4 deletions packages/dom/src/serialize-dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,19 @@ function doctype(dom) {
return `<!DOCTYPE ${name}${deprecated}>`;
}

// Un-mangle both serialization markers in one pass instead of two:
// <data-percy-custom-element-x> -> <x>
// ` data-percy-serialized-attribute-y=` -> ` y=`
// 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;

@rishigupta1599 rishigupta1599 Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

Suggested change
const PERCY_MARKER_RE = /(<\/?)data-percy-custom-element-|( )data-percy-serialized-attribute-(\w+?)=/gi;
const PERCY_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)


// Serializes and returns the cloned DOM as an HTML string
function serializeHTML(ctx) {
let html = getOuterHTML(ctx.clone.documentElement, { shadowRootElements: ctx.shadowRootElements, forceShadowAsLightDOM: ctx.forceShadowAsLightDOM });
// this is replacing serialized data tag with real tag
html = html.replace(/(<\/?)data-percy-custom-element-/g, '$1');
// replace serialized data attributes with real attributes
html = html.replace(/ data-percy-serialized-attribute-(\w+?)=/ig, ' $1=');
html = html.replace(PERCY_MARKER_RE, (_match, tagPrefix, attrSpace, attrName) =>
tagPrefix !== undefined ? tagPrefix : `${attrSpace}${attrName}=`);
// include the doctype with the html string
return doctype(ctx.dom) + html;
}
Expand Down
Loading