Repository navigation
fix(core): bound ASCII folding allocations for large HTML - #5098
Conversation
There was a problem hiding this comment.
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,insertBeforeCloseTagandinjectTagsAtHeadStartall finish in 1.7–5.6 s. Control: with main'shtmlDocument.ts,stripandinjecthitFATAL 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 fulltoLowerCase()(the İ offset case). - Tests:
htmlDocument.test.tspasses 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
Edit accuracy: accurate 2055 (base branch 2055), smooth 1598 of thoseThe gate passes. Quarantined, measured but not gated (0) |
There was a problem hiding this comment.
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.replacereturns the input when there are no ASCII uppercase matches; the new loop builds a full-size concatenated string even then.stripEmbeddedRuntimeScriptsfolds 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 withFATAL 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=tsxpasses 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)
|
At
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. |
|
@terencecho Addressed in 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 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
left a comment
There was a problem hiding this comment.
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
.replacereturns 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.
- 48 MiB all-lowercase authored script at a 96 MiB old-space: passes on this head, OOMs on
- 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.tspasses 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
left a comment
There was a problem hiding this comment.
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
stripEmbeddedRuntimeScriptswitness 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)
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
lowerAsciihelper 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:
String::SlowFlatten, and the revision passes.These are fixed-fixture heap checks, not a constant-total-memory guarantee. The original triggering input is unknown.