Repository navigation
perf(parsers): ensureHfIds skips the parse when every element already has its id - #7069
Conversation
There was a problem hiding this comment.
Request changes at 7cd5c0f9: one correctness blocker, and the fix is one line.
Blocker: in a fragment, alreadyStamped skips every element before a <body> tag
When the input is a fragment (isFragment is true), alreadyStamped still slices from the first body start tag it finds. Elements before that tag are never checked, and the body tag itself is never tested against IMPLIED. The wrapped parse on main keeps all of them inside the body and mints ids for them, so the fast path returns the input without ids that main would have added. That breaks the PR's own invariant ("a repeated html/head/body … answers false").
I diffed main against this head, comparing the minted id sets for the same input:
| input | main mints | this PR |
|---|---|---|
<head><title>t</title></head><body><div data-hf-id="a">x</div></body> (an HTML page that leaves out <html>, which is valid) |
head, title, body |
nothing (fast path) |
<style>.a{}</style><section>hi</section><body><div data-hf-id="a">x</div></body> |
section, body |
nothing (fast path) |
<p>y</p><body><div data-hf-id="a">x</div> |
p, body |
nothing (fast path) |
Suggested fix, which I checked locally:
const body = isFragment(html) ? -1 : tags.findIndex((tag) => tag.name === "body");With this change, a body tag inside a fragment hits IMPLIED and takes the full parse. All three cases above then match main, and hfIds.test.ts plus hfIds.parseCount.test.ts pass (44/44). Please add one of these cases to hfIds.parseCount.test.ts (it should parse once and mint).
Verified otherwise
hfIds.parseCount.test.tsfails on main: the stamped case gets one parse and a re-serialized string. It passes at this head.- Parsers package: everything passes except
htmlParser.roundtripandstableIds. Those two also fail on main in my sandbox because@hyperframes/core/generatorsisn't built there, so the failure isn't caused by this PR. - The rest of my main-vs-PR probe matched: composition and plain templates, CDATA in SVG,
textarea/noscript/plaintext/iframe, comments containing tags, uppercase or unquotedDATA-HF-ID, a whitespace-only id,select/option, MathML, content after</html>. - Returning the raw input instead of linkedom's serialization is fine for the callers I checked (
preview.ts,sourceMutation.parseSourceDocument, sdkparseMutable/buildDocument). It also stopstimeline idsfrom rewriting files that are already stamped just because the serialization normalizes them.
Note: this is a perf PR, not a fix, so I didn't hold it to the fails without the fix: line. The parse-count test does fail on main.
|
Right, thanks for the three cases. Fixed at Added your |
There was a problem hiding this comment.
Request changes at bb8f1a5f. The fragment fix is right. The remaining blocker is a red CI check that comes from this PR.
Resolved: the fragment <body> slice
alreadyStamped now reads a fragment from its first tag, so a stray <body> hits IMPLIED and takes the parse. The new test, "checks a fragment from its first tag…", fails against the previous head 7cd5c0f9 and passes here. All three cases from my earlier review now match main. The parsers suite passes in full (1395 passed; the two files that failed before only needed core built). oxfmt --check is clean.
Blocker: the Comments check fails
RAISED comment share: packages/parsers/src/hfIds.ts is 6.1% (15/245), at the base 4.2% (9/213)
6 comment line(s) added. Cut them, or move the explanation into a name or a type.
This check also failed at 7cd5c0f9, which I missed in the first review. To get back to ≤4.2%, hfIds.ts can keep at most about one of the six added comment lines. Candidates to cut:
- the
// A bundle stamped at build time…line inensureHfIds; - the new
// A fragment's every tag is body content…line; - the three-line
alreadyStampeddocblock; - the
hasHfIddocblock.
The names, together with the parse-count test descriptions, already carry most of what those lines explain. Cutting the six lines brings the file to 9/239 (3.8%).
I'll approve once the Comments check passes on the new head.
bb8f1a5 to
f052915
Compare
|
At Fix: the fast path now also needs the page to open |
There was a problem hiding this comment.
Request changes at f0529151. The logic is sound. The blocker is formatting, and it fails CI again.
Blocker: hfIds.parseCount.test.ts fails oxfmt --check
On this head, Preflight (lint + format) fails in both Windows render verification and preview-regression, and the downstream Windows Tests/Render and preview-regression "Check results" failures follow from it. The log says packages/parsers/src/hfIds.parseCount.test.ts … Format issues found. The cause is the second ensureHfIds(...) call in the new "parses a stamped page not opening then " test (line 46), whose line is too long. oxfmt wants:
ensureHfIds(
`<!DOCTYPE html><html><head><title>t</title><body><p data-hf-id="hf-a1b2">x</p></body></html>`,
);Run npx oxfmt on that file. No logic change is needed.
Verified
- The Comments ratchet blocker from my last review is resolved: the check passes, and five of the six added comment lines are gone.
opensHeadThenBodyis conservative in the right direction. It requires<html>, then<head>, then only head-content tags, then a</head>(outside comments) before<body>. The</head>search starts after the last head tag's raw-text end, so a</head>inside a<script>/<style>/<title>doesn't count. Anything else takes the parse as before.- The fragment fix and its test are unchanged from
bb8f1a5f. - Locally at this head, the parsers suite passes (1396 passed) and the SDK suite passes (709/709, including the round trips).
I'll approve once a formatted head passes preflight.
There was a problem hiding this comment.
Approve at 9c9941a1. No blocker remains.
- Formatting blocker resolved:
oxfmt --checkis clean on both files. Every workflow on this head passes: CI, Comments, CodeQL, preview-regression, Windows render verification, regression, PR captures and Player perf. closesHeadsplit: it behaves the same as before for well-formed input, and it now scans comments and</head>in a single left-to-rightmatchAll. A</head>inside a comment is consumed by the comment match and doesn't count, which resolves CodeQL's comment-replace finding. When the last head tag's raw text never ends (from < 0), it answers false. The new shape cases (unclosed head, a<div>in head, no body, a commented-out</head>) each take the parse.- Local runs at this head: parsers 1396 passed, SDK 709/709.
- Carried forward: the fragment fix, the head-then-body gate, and the parse-count test that fails on main are all unchanged from what I verified on earlier heads.
What
ensureHfIdsreturns its input as it is, without a parse, when every element it would give an id already has a non-emptydata-hf-id.Why
The studio preview build calls
ensureHfIdstwice on the bundled document, and the bundle is already stamped (stampHfIds: true), so both calls parse and re-serialize the whole page to mint nothing. On a page with a large inline SVG that is most of the build's time in the server process, which blocks it for as long as it takes.On a 20,000-path inline SVG (about 1.4 MB once stamped), one call on the stamped document takes a median of 249 ms on main and 33 ms with this change, so a preview build of such a page does about 430 ms less work.
Related work
The first step of making the preview build parse its document less often. The bundler and the other preview steps still parse on their own; they come next.
How
alreadyStamped(html)reads the start tags with the file's existing tag reader (startTags, the onestampHfIdsInPlaceuses), from the<body>tag on (or the whole text for a fragment), and answers true only when each one is inEXCLUDED_TAGSor carries a non-emptydata-hf-idamong its own attributes.hasHfIdreads attributes with quoted values masked (outsideQuotes), so an id-looking string inside another attribute's value never counts.<html>with<head>(holding onlymeta,link,base,title,style,script) then</head>and<body>. The parse path'sdocument.bodyread makes linkedom add a missing<head>or<body>, so its output always has both, and callers that re-parse it rely on that (the SDK's edit-and-undo round trips compare serialized documents);<body>(a fragment is checked from its first tag);<table>, whose parse can add elements the text has none of;html/head/body;<template>without an id.assignHfIdsonly mints ids for elements without one and never changes an existing id, so skipping it when nothing is missing changes no id.Test plan
hfIds.parseCount.test.tscounts parses. A fully stamped page comes back unchanged with no parse (fails on main: one parse, and the re-serialized text). A missing or empty id still parses once and mints, a stamped page without its<head>(or without</head>) still parses and gets both, elements before a stray<body>in a fragment get their ids, and an id inside a quoted value does not count.<head>on an earlier head of this branch); lint, format,tscand the comment share pass.Size
About 85 lines (half of them the test): the first, self-contained step of a larger cut. The preview build's other parses (the bundler's and the later preview steps') are separate changes in other packages, each with its own parse-count test; folding them in would mix three packages in one review.