Skip to content

fix(core): bound ASCII folding allocations for large HTML - #5098

Merged
miguel-heygen merged 3 commits into
mainfrom
fix/bounded-html-ascii
Oct 6, 2026
Merged

miguel-heygen merged 3 commits into
mainfrom
fix/bounded-html-ascii

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

A large bundled HTML document can exhaust V8 memory when ASCII folding collects callback matches across the entire input. Unconditionally concatenating smaller chunks also creates unnecessary full-document copies for lowercase authored scripts.

The shared lowerAscii helper now replaces spans of at most 64 KiB beginning at uppercase ASCII. Inner match collection stays bounded, unchanged spans retain native replacement behavior, and all HTML document helpers keep ASCII-only conversion and UTF-16 offsets.

Validation on Linux with Node 22.22.1:

  • 32 MiB mixed-case base64 under a 256 MiB old-space cap: the original whole-input replacement fails; the revision preserves media and removes the runtime.
  • 48 MiB lowercase authored script under 96 MiB old-space and 4 MiB semi-space: the original base passes, the previous PR head fails in String::SlowFlatten, and the revision passes.
  • 48 MiB single-uppercase authored script passes under 128 MiB old-space and 4 MiB semi-space. The 96 MiB variant also fails the original base and is not claimed as a regression witness.
  • UTF-16 offsets survive a split surrogate pair, Unicode inside a transformed span, and mixed-case tags crossing a 64 KiB boundary.
  • All 55 helper tests pass three consecutive runs, without skips. Deliberate whole-input replacement, unconditional chunk concatenation, and Unicode-folding mutations are rejected by their witnesses.
  • Core build, core and runtime typechecks, changed-file lint, formatting, and comment gates pass. Independent review found no blockers.

These are fixed-fixture heap checks, not a constant-total-memory guarantee. The original triggering input is unknown.

@miguel-heygen
miguel-heygen marked this pull request as ready for review October 6, 2026 01:17

@somanshreddy somanshreddy left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review at 8b2e874d (full PR). This is a comment, not an approval.

No blockers. The chunked lowerAscii gives the same output as the old version: the fold works one UTF-16 unit at a time, so cutting the input at fixed offsets (even through a surrogate pair or an [A-Z]+ run) can't change the result. All 10 lowerAscii call sites in htmlDocument.ts go through it.

What I checked

  • Every public entry point, 256 MiB heap, 32 MiB mixed-case base64 page (uppercase tags, runtime script present). On the head, stripEmbeddedRuntimeScripts, injectScriptsIntoHtml, insertRuntimeTag, insertBeforeCloseTag and injectTagsAtHeadStart all finish in 1.7–5.6 s. Control: with main's htmlDocument.ts, strip and inject hit FATAL ERROR: Reached heap limit.
  • Same output and faster. Old and new produce identical strings on a 4 MiB typical page and an 8 MiB base64 page. Without a heap cap the new version is quicker: 157→83 ms on the typical page and 638→396 ms on the base64 page.
  • Mutations: 5 of 5 caught by htmlDocument.test.ts: main's version, a 64 MiB chunk, a slice one unit short, a step one unit long, and a full toLowerCase() (the İ offset case).
  • Tests: htmlDocument.test.ts passes 53/53 locally. The new boundary test does split the 😀 surrogate pair at 65 536.

Info (not this PR): each helper lowers the whole document again (inject takes about 5.6 s on 32 MiB), so a large page pays the cost several times per compile. This was already the case on main.

CI at the head: 79 pass, 4 pending, 8 skipped, 0 failing (rechecked later; the first post said 38 pass, 20 pending).

— Somu

@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Edit accuracy: accurate 2055 (base branch 2055), smooth 1598 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@terencecho terencecho left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

At 8b2e874dd4538b83c8f0e1d011169476022a278c: the 32 MiB mixed-case bounded-heap test exercises the original V8 failure, and the chunk-boundary test checks UTF-16 offsets across a surrogate pair (packages/core/src/compiler/htmlDocument.test.ts). I found one inverse-case blocker:

  • Blocker — large lowercase authored scripts now exhaust the heap (packages/core/src/compiler/htmlDocument.ts:37–41). The old whole-input .replace returns the input when there are no ASCII uppercase matches; the new loop builds a full-size concatenated string even then. stripEmbeddedRuntimeScripts folds the full HTML at line 47 and folds an authored script block again at line 163; the resulting rope/flatten allocations make the memory fix fail for a different valid large-HTML shape. With Node 22 and --max-old-space-size=128 --max-semi-space-size=16, a 48 MiB lowercase string in <script>const embeddeddata="aaa…";</script> passes through the actual exported strip helper with real linkedom: the PR base returns the unchanged HTML (exit 0, ~102 MiB heap after the call), while this head aborts with FATAL ERROR: Reached heap limit (exit 134), three runs out of three. This is GC-budget- and loader-sensitive: reducing the semi-space to 1–8 MiB makes both versions pass at 48 MiB, and direct --import=tsx passes both at 48 MiB. With the same Node flags and direct TSX source loading, a 60 MiB lowercase authored script instead passes on the base and OOMs at the head (three of three runs). Preserve the no-uppercase fast path and avoid retaining/flattening multiple full-document copies for mostly lowercase input; add a bounded-heap test for an authored data-heavy script alongside the mixed-case regression.

Verdict: REQUEST CHANGES
Reasoning: The new chunk-concatenation path introduces a reproducible head-only OOM through a real public caller on large valid HTML, the failure class this PR is intended to fix.

— Review by tai (pr-review)

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

At 8b2e874dd4538b83c8f0e1d011169476022a278c, I compared the actual exported stripEmbeddedRuntimeScripts with the PR base 568dc2ae using Node 22.22.1 and real linkedom.

  • With --max-old-space-size=128, both versions preserve a 48 MiB lowercase authored script and its single-uppercase variant. Both also pass when that script is surrounded by a full document with mixed-case markup. These runs use --import=tsx --input-type=module; the full-document runs ended around 105 MiB heap used.
  • A tighter budget, --max-old-space-size=96 --max-semi-space-size=4, does expose a head-only regression for the all-lowercase full document: base passes, head aborts in String::SlowFlatten during lastIndexOf. The single-uppercase variant fails on both base and head at that tighter budget, so it is not evidence of a new regression by itself.

The no-op allocation concern is supported by that narrower witness. I have not pushed a revision while reconciling the original reproduction. Could you share the exact fixture construction, complete Node command/version, and module-loading setup for the 128 MiB failure? That will let me validate the change against the same failure rather than substitute a different memory envelope.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

@terencecho Addressed in 3df919fe2b822610fe9a8ff872d840a4ffe727d9.

The shared helper now replaces at most 64 KiB beginning at uppercase ASCII, instead of concatenating every chunk. Lowercase input uses native no-match behavior, while inner callback match collection remains bounded. ASCII-only folding still preserves UTF-16 offsets.

Committed the proven regression: a 48 MiB lowercase authored script with --max-old-space-size=96 --max-semi-space-size=4 passes the original base, fails 8b2e874d in String::SlowFlatten, and passes this revision. The single-uppercase 48 MiB case is also covered at 128 MiB; its 96 MiB failure occurs on the original base too. I am not claiming reproduction of the original 128 MiB failure.

All 55 helper tests passed three consecutive Linux/Node 22.22.1 runs, including the 32 MiB mixed-case allocation witness and the 64 KiB Unicode/tag-boundary witness. Deliberately restoring the old chunk loop reproduces the lowercase crash; the original whole-input replacement fails the dense fixture; Unicode folding fails the offset witness. Core build/typechecks, scoped lint/formatting, comment gates, and independent source review passed.

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-check at 3df919fe (delta from 8b2e874d; it's a direct descendant). This is a comment, not an approval.

Correction to my earlier review: I tested only mixed-case input and missed the all-lowercase case Terence's change request found. My chunked loop at 8b2e874d built a full copy even when there was nothing to fold. Credit to Terence.

No blockers at this head. lowerAscii now matches [A-Z][\s\S]{0,65535}, so each chunk starts at an uppercase letter:

  • Text with no uppercase letters gets no matches, so .replace returns the input unchanged and nothing is copied.
  • Mixed-case text produces at most about len/64 Ki matches, so the callback match array stays bounded.
  • Correctness: the text between matches never contains A–Z, and each match is folded in full.

What I ran

  • Equivalence fuzz: compared against a per-character ASCII fold on 300 random strings. These used lengths at and across the 64 Ki boundary, lone surrogates, İ, ß and the Kelvin sign. 0 mismatches, and the lengths are preserved.
  • Heap probes:
    • 48 MiB all-lowercase authored script at a 96 MiB old-space: passes on this head, OOMs on 8b2e874d.
    • 32 MiB mixed-case page at a 256 MiB heap, through all 5 public htmlDocument entry points: all 5 still pass.
  • Speed: 4 MiB typical page 96 ms, 8 MiB base64 page 437 ms, 48 MiB lowercase 61 ms. This is in line with the previous head.
  • Tests: htmlDocument.test.ts passes 55/55.
  • Mutations: 4 of 4 caught. Previous head, a chunk that starts at any character, a 4-character chunk, and a lazy quantifier.

Info: input that really is uppercase or mixed-case still needs a full lowered copy beside the input. At 48 MiB that didn't fit in 160 MiB on this head or the previous one. That's inherent to the fold and not a regression.

CI at this head: 57 pass, 30 pending, 6 skipped, 0 failing so far. Terence's change request at 8b2e874d still stands; it's his to clear.

— Somu

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review at 3df919fe2b822610fe9a8ff872d840a4ffe727d9 of my CHANGES_REQUESTED review at 8b2e874dd4538b83c8f0e1d011169476022a278c:

  • Lowercase heap blocker resolved (packages/core/src/compiler/htmlDocument.ts:33–38): the outer replacement now starts only at an ASCII uppercase character and bounds each matched span to 65,536 UTF-16 units. With no uppercase character, V8 returns the original input unchanged rather than constructing a chunk-concatenation rope. The inner replacement still folds only ASCII uppercase.
  • I reran the real stripEmbeddedRuntimeScripts witness with a 60 MiB lowercase authored script through direct TSX under Node 22 (--max-old-space-size=128 --max-semi-space-size=16): the prior reviewed source exits 134 with heap OOM; this head returns the identical HTML and exits 0. The added red-first 48 MiB test fails on the previous head at 96 MiB old space / 4 MiB semi-space and passes here. A single-uppercase 48 MiB fixture passes under its 128 MiB cap; the 32 MiB mixed-case runtime-removal fixture also passes under 256 MiB.
  • UTF-16 offsets, surrogate/İ behavior and mixed-case tag searches remain intact across chunk boundaries in the changed tests and an independent randomized comparison. All available public helper paths retained expected output in the probes. I did not rerun the full Vitest suite locally; the at-head CI Test check passed.

No remaining code-merit blocker found. CI still has 20 Studio edit-accuracy shards running at review time, so approval is not a claim that the merge gate is clear.

Verdict: APPROVE (replaces my prior changes-requested verdict).

— Review by tai (pr-review)

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 6, 2026
Merged via the queue into main with commit 00342fc Oct 6, 2026
169 checks passed
@miguel-heygen
miguel-heygen deleted the fix/bounded-html-ascii branch October 6, 2026 04:29
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.

3 participants