Skip to content

perf(rendering): batch node-label measurement + construction - #7872

Closed
knsv-bot wants to merge 23 commits into
developfrom
perf/batch-label-measure
Closed

knsv-bot wants to merge 23 commits into
developfrom
perf/batch-label-measure

Conversation

@knsv-bot

@knsv-bot knsv-bot commented Jun 17, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

The steady-state (warm) win: batch the two most expensive per-node operations in
the measure phase — size measurement and label DOM construction — plus the
profiling that pinpointed them. No visual change (cypress snapshots clean).
Stacked on the surgical-reflow PR.

Validated results (dagre folder benchmark, trimmed mean)

baseline this PR gain
set measure 3010 ms 2491 ms −17%
huge1 measure 190 ms 104 ms −45%
huge1 total render 427 ms ~340 ms ~−20%

Flat / label-heavy flowcharts (the common case) gain −39 to −49% of measure.
Subgraph-heavy diagrams gain less — they're bound by nested dagre layout, not DOM.

Changes

  1. Batched measurement (prebuildNodeLabels in shapes/util.ts, wired into the
    common createGraphWithElements and dagre measureDagreGraph): build every label
    first (deferring its getBoundingClientRect), then read all sizes back-to-back so
    only the first forces a reflow.
  2. Batched construction (createText.ts htmlLabelMarkup + util.ts
    buildNodeLabelsBatched): assemble the markup for all HTML labels into one string
    and parse it with a single insertAdjacentHTML — the HTML parser builds the
    foreignObject/div/span sub-trees ~2× faster than per-element createElement
    (measured: labelBuild −33% set, −49% on flat diagrams).
    • Safety: a one-time probe + per-render structure check; if SVG-markup parsing
      ever misbehaves (e.g. jsdom), it falls back to per-element construction, so
      correctness never depends on the parser.
  3. Measure-phase profiling buckets (sanitize/markdown/decode/labelBuild)
    that decompose the phase — how each win above was found and proven. Guarded by
    injected.profiling, tree-shaken from production.

Note on the dagre→common-pipeline migration

This rebases cleanly onto that migration: the dagre measure span is dropped
(redundant — the common renderer's measure span now wraps measureDagreLayout),
while the dagre batching stays and runs inside that measure phase via the new
measureDagreGraph.

Testing

Unit tests pass (the probe + fallback keep jsdom green); cypress visual snapshots
clean
; zero new type errors.

knsv and others added 8 commits June 17, 2026 09:09
The per-node measure loop builds a label's DOM and immediately reads its size
(getBoundingClientRect / getBBox), interleaved across all nodes — so each read
forces a synchronous reflow over the growing tree. On a large dagre flowchart
this was ~2,216 Layout events / 173 ms (45% of the measure phase).

Split label handling into build -> measure -> finalize, and add
prebuildNodeLabels: build every label first (DOM writes only; the HTML path now
defers its internal getBoundingClientRect via createText's `deferMeasure`), then
read all sizes back-to-back so only the first read forces a reflow, then write
the positions. labelHelper returns these prebuilt labels; anything not prebuilt
(groups, linked nodes, shapes that build their own label) falls back to the
inline build+measure path unchanged, so this is purely an optimization.

Wired into createGraphWithElements (the common/ELK measure path). The dagre
recursiveRender path is a separate follow-up.

NOTE: jsdom returns 0 for getBoundingClientRect/getBBox, so unit tests cover
structure only — the actual reflow reduction and measured-size correctness must
be validated in real Chrome (cypress visual snapshots + the Dev Explorer
profiler).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Apply the same build-all-then-measure-all batching to the dagre layout path,
which measures nodes inside recursiveRender (its measureLayout hook is a no-op).
Before each recursion level's node loop, prebuild + batch-measure the level's
leaf-node labels; the loop's insertNode calls then consume the prebuilt labels,
collapsing the per-node label reflow to roughly one per recursion level.

clearPrebuiltLabels now takes the specific nodes a prebuild produced, so nested
concurrent subgraph renders clear only their own pending entries.

Same validation caveat as the common-path change: jsdom can't measure, so the
reflow reduction and size correctness need real-Chrome (cypress + profiler)
verification. This is the path the original trace profiled.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The warm benchmark shows getBBox/getBoundingClientRect (the reflow reads) are
only ~4% of the measure phase — the rest is unattributed DOM construction. Add
two buckets to find it:
 - sanitize: sanitizeMore + DOMPurify cost (per sanitizeText call).
 - labelBuild: the full label DOM construction (createText) in the prebuild loop.

Both guarded by injected.profiling. sanitize uses tickSync (sync, so accurate
under concurrency); labelBuild uses the async tick in the sequential prebuild
loop, where wall-clocks don't overlap. Diagnostic instrumentation to decide
where the warm measure cost actually is (DOMPurify vs DOM building vs shapes).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a `markdown` bucket around markdownToHTML/nonMarkdownToHTML so labelBuild
can be split into markdown parsing vs element creation:
  element-creation ≈ labelBuild − markdown − sanitize.
This decides whether a plain-text markdown fast-path is worthwhile or the cost
is in DOM element construction. tickSync (sync) so it's accurate under the
concurrent label builds.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add labelPrep (decodeEntities + replaceIconSubstring regex) and addHtmlSpan
(foreignObject/div/span DOM construction) buckets so the ~90%-of-labelBuild
"element-creation" cost can be attributed to regex prep vs actual DOM building —
deciding between a trivial plain-label fast-path and a DOM-construction rewrite.
Async tick, accurate in the sequential prebuild loop.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ync decode

The labelPrep/addHtmlSpan buckets used the async `tick`, which over-counts when
createText runs concurrently (recursion/clusters/fallback) — they reported
physically impossible values (150k+ ms). Drop them. decodeEntities is sync, so
measure it with tickSync (accurate under concurrency); replaceIconSubstring is a
no-op scan for plain labels (negligible). Confirms regex prep is not the cost —
element-creation is DOM construction in addHtmlSpan.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…HTML

Label element-creation (foreignObject/div/span per node) is ~50% of the warm
measure phase on large flowcharts, and a console A/B showed the HTML parser
builds these sub-trees ~2x faster than the DOM API. When every prebuilt label is
HTML, assemble the markup for all of them into one string and parse it with a
single insertAdjacentHTML instead of per-element createElement/setAttribute.

- createText: extract prepareHtmlLabelNode (shared text pipeline) and add
  htmlLabelMarkup (string form of addHtmlSpan, identical output) + escapeAttr +
  registerDeferredHtmlLabel.
- util.ts: prebuildNodeLabels routes through buildNodeLabelsBatched when a
  one-time probe confirms the environment parses batched SVG/xhtml markup with
  correct namespaces; otherwise (e.g. jsdom) falls back to per-element
  construction. A per-render structure check undoes and falls back if the parse
  is ever unexpected, so correctness never depends on the parser.

Validation: jsdom unit tests green via the fallback/probe; the batched path's
visual correctness must be confirmed by the cypress snapshot suite, and the win
by re-benchmarking huge1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Jun 17, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 48b82a9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
mermaid Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov

codecov Bot commented Jun 17, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.64730% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.81%. Comparing base (388ccd0) to head (48b82a9).
⚠️ Report is 110 commits behind head on develop.

Files with missing lines Patch % Lines
...c/rendering-util/rendering-elements/shapes/util.ts 81.45% 6 Missing and 17 partials ⚠️
packages/mermaid/src/rendering-util/createText.ts 86.79% 1 Missing and 6 partials ⚠️
packages/mermaid/src/rendering-util/createGraph.ts 73.33% 0 Missing and 4 partials ⚠️
packages/mermaid/src/diagrams/common/common.ts 75.00% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #7872      +/-   ##
===========================================
+ Coverage    67.78%   67.81%   +0.02%     
===========================================
  Files          429      429              
  Lines        38648    38893     +245     
  Branches      6380     6435      +55     
===========================================
+ Hits         26199    26376     +177     
- Misses        9675     9724      +49     
- Partials      2774     2793      +19     
Flag Coverage Δ
e2e 70.35% <84.18%> (+0.08%) ⬆️
unit 24.35% <75.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...rc/rendering-util/layout-algorithms/dagre/index.js 92.97% <100.00%> (+0.13%) ⬆️
.../rendering-elements/shapes/handDrawnShapeStyles.ts 95.00% <100.00%> (+0.17%) ⬆️
packages/mermaid/src/diagrams/common/common.ts 57.20% <75.00%> (-0.13%) ⬇️
packages/mermaid/src/rendering-util/createGraph.ts 77.77% <73.33%> (-1.39%) ⬇️
packages/mermaid/src/rendering-util/createText.ts 91.35% <86.79%> (-2.45%) ⬇️
...c/rendering-util/rendering-elements/shapes/util.ts 82.83% <81.45%> (-0.77%) ⬇️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@argos-ci

argos-ci Bot commented Jun 17, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ✅ No changes detected - Jun 22, 2026, 11:51 AM

knsv and others added 3 commits June 17, 2026 15:09
# Conflicts:
#	packages/mermaid/src/rendering-util/rendering-elements/shapes/util.ts
… color)

prebuildNodeLabels builds and measures leaf-node labels up front, but icon/image
shape handlers set node.labelStyle (the classDef-derived label color), override
node.width, and pass a shape-specific label class ('icon-shape'/'image-shape') to
labelHelper *after* the prebuild pass. A prebuilt icon/image label was therefore
built and measured with the wrong class, stale style and stale width — dropping
the label's padding and its classDef color (e.g. white text rendered black).

Exclude node.icon/node.img from the prebuild filter so they fall back to the
inline build path inside their handler. Regular shapes keep the batched prebuild:
they pass getNodeClasses(node) and don't override width, matching what prebuild
uses, so they're unaffected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- updateNodeBounds: guard against degenerate (zero/negative) knownBounds so it
  falls back to a real getBBox() measurement instead of committing the node to
  0×0.
- Downgrade the stray `log.warn('Returning from recursive render XAX', …)` debug
  trace to `log.debug` (the expensive 'Graph after XAX' graph-serialization log
  was already commented out).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Base automatically changed from perf/measure-phase-reflows to develop June 18, 2026 07:54
Two whitespace-related regex pathologies made a deeply-indented flowchart (e.g.
the perf fixture huge3, with 1.6k-space lines) cost ~1.4s to parse — both
O(whitespace²):

- anyCommentRegex (diagram-api/regexes.ts), used by detectType + preprocess: the
  leading greedy \s* + global flag re-scanned each indent run from every position.
  Guard the start with (?:^|(?<=\S)) so \s* is attempted once per run — O(n), and
  byte-identical output (new regexes.spec.ts asserts equivalence vs the legacy
  pattern over a corpus + an O(n) regression guard).
- flow.jison SPACE rule matched a single whitespace char (\s), emitting one SPACE
  token per space — hundreds of thousands on deep indentation. Match a run
  ([^\S\n\r]+) instead; the grammar only uses SPACE as a separator, so parsing is
  unchanged (full flowchart suite green, 991 tests).

huge3 Diagram.fromText: ~1410ms -> 27ms.
@netlify

netlify Bot commented Jun 18, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 48b82a9
🔍 Latest deploy log https://app.netlify.com/projects/mermaid-js/deploys/6a391d03c647010008464bcf
😎 Deploy Preview https://deploy-preview-7872--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@pkg-pr-new

pkg-pr-new Bot commented Jun 18, 2026 •

Copy link
Copy Markdown

Open in StackBlitz

@mermaid-js/examples

npm i https://pkg.pr.new/@mermaid-js/examples@7872

mermaid

npm i https://pkg.pr.new/mermaid@7872

@mermaid-js/layout-elk

npm i https://pkg.pr.new/@mermaid-js/layout-elk@7872

@mermaid-js/layout-tidy-tree

npm i https://pkg.pr.new/@mermaid-js/layout-tidy-tree@7872

@mermaid-js/mermaid-zenuml

npm i https://pkg.pr.new/@mermaid-js/mermaid-zenuml@7872

@mermaid-js/parser

npm i https://pkg.pr.new/@mermaid-js/parser@7872

@mermaid-js/tiny

npm i https://pkg.pr.new/@mermaid-js/tiny@7872

commit: 48b82a9

knsv and others added 3 commits June 18, 2026 10:18
…rebuild

prebuildNodeLabels builds and measures each leaf node's label up front, then
labelHelper hands that prebuilt label back to the shape. But several shapes
mutate the node *after* the prebuild pass runs:

- hourglass blanks node.label -> the prebuilt label still showed the text
- erBox rewrites node.label to the entity alias and bumps node.width -> the
  prebuilt label showed the id and was sized/positioned wrong (title missing)
- shapes apply a classDef-derived node.labelStyle via styles2String -> the
  prebuilt label was built with the un-styled labelStyle, dropping label colour

labelHelper now stamps each prebuilt label with a cheap signature of the node
state it was built from (label, width, markdown, labelStyle, html-labels,
centerLabel, icon/img) and only reuses it when the node still matches. When a
shape has customised the node the signature differs, so we drop the stale
prebuilt DOM and rebuild inline -- exactly the pre-perf path. Unchanged plain
nodes (the bulk of large diagrams) still take the fast prebuilt path.

Also run configureLabelImages on the batched markup build path: like the
per-element path it must wait for <img> labels to load and apply their sizing
styles before measuring, otherwise image nodes are mis-sized and the images
don't show.

Repro fixtures added under cypress/platform/dev-diagrams/render-regressions/.
A prebuilt leaf-node label is inserted into the nodes group during the prebuild
pass, before any shape runs. erBox draws a hand-drawn background rect as a
sibling g first and then builds the entity, relying on the entity's g being
appended after the background so it paints on top. When labelHelper reused a
prebuilt label in place, it stayed at its earlier position, so the background
painted over the entity and hand-drawn ER entities lost their background fill.

labelHelper now raises the reused g to the end of its parent, matching the
inline path (which appends the label at shape-render time). Short-named ER
entities were already correct because their width bump flips the label
signature and forces an inline rebuild; this covers the rest.

Repro fixture: er-handdrawn-entity-background.mmd.

@knsv-bot knsv-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[sisyphus-bot]

Thanks for this — the build-all-then-measure-all batching is a genuinely smart attack on the per-node forced-reflow cost, and the engineering care shows. Since it's a draft, this is early-direction feedback, not a merge gate; nothing below is blocking. I read the full diff at head 501fb49, traced the new string-HTML path, ran an XSS pass, and read the regression-fixtures README. (Heads-up: the pre-fetch flagged this as a "new diagram" — false positive; it's a perf refactor, so the new-diagram checklist doesn't apply.)

What's working well

  • 🎉 The batching architecture is the right shape. Splitting labelHelper into buildNodeLabel (writes) → measureNodeLabel (the single forced read) → finalizeNodeLabel (writes), so a batch runs every read back-to-back, is exactly how you collapse the reflow storm. The validated numbers (−17% / −45% measure) back it up.
  • 🎉 The correctness guards are thoughtful, not hand-wavy. The labelSignature fingerprint to detect post-prebuild node mutation (hourglass blanking the label, erBox rewriting width, classDef-derived labelStyle) and fall back to the exact inline path; the canBatchSvgHtml() probe with per-element fallback so correctness never depends on the parser; clearPrebuiltLabels(nodes) scoped to avoid clobbering concurrent nested subgraph renders; the raise() to preserve paint order; and the degenerate-bounds guard in updateNodeBounds — these are the subtle pitfalls of caching+reordering DOM, and you've handled each. The commit history (icon/image padding, post-prebuild rebuild, paint order) shows they were found and fixed deliberately.
  • 🎉 regexes.spec.ts is a model equivalence test — keeping the legacy pattern as a byte-for-byte oracle over a whitespace-edge corpus plus a backtracking-perf assertion is exactly how to make a regex micro-opt reviewable.
  • 🎉 The render-regressions README is honest and useful — mapping each repro to the specific Cypress/Argos test that catches it is great traceability.

Things to consider (for when you take it out of draft)

🟡 "No visual change" vs. a folder of 8 known regressions — please confirm each is resolved on head

This is the whole correctness bar for the PR, so it's worth being explicit. The README documents 8 rendering regressions this perf work introduced (note text wrapping differently, hourglass rendering a label it shouldn't, ER entity titles vanishing, <img> icons not showing, box-label alignment flips, SVG markdown wrapping at different words). The commits fix several — but the README frames them as "compare against develop to confirm the regression," which reads as open. Before un-drafting, it'd be great to confirm every listed Argos test is green on head (especially imageshape-markdown-svg-wrap and class-notes-simple-alignment, which are wrap/alignment shifts the batched-markup path can reintroduce), and reword the README so it's clear these are fixed repros, not outstanding ones.

🟡 Scope: two unrelated perf concerns are bundled

The headline (label batching) and the flowchart parser O(n²) fix (flow.jison SPACE token + anyCommentRegex, with the flowchart-deep-indent-parse-perf changeset and regexes.spec.ts) are independent — different subsystem, different changeset, different tests. The parser fix is cleanly separable and would be much easier to land and bisect on its own. Worth splitting it into its own PR (it also looks ready/lower-risk than the batching, so it could land first).

🟡 The jison SPACE change needs the full flowchart parser suite green

\s → [^\S\n\r]+ (one token per whitespace run instead of per char) is a lexer grammar change. The "grammar only uses SPACE as a separator, so parsing is unchanged" claim is plausible, but jison changes are regression-prone and don't hot-reload (dev server restart needed). Please confirm the flowchart .spec suite passes. One edge to sanity-check: a lone \r not followed by \n used to match \s as SPACE and now matches nothing in that rule — almost certainly irrelevant, but worth a thought.

💡 Changeset coverage

The only changeset is for the bundled parser fix. The headline label-batching change has none — for a user-affecting perf improvement touching shared rendering-util/ I'd add a patch changeset (or, if you split per the point above, one per PR).

🟢 anyCommentRegex lookbehind

(?:^|(?<=\S)) uses a lookbehind — fine on current Node/browsers, just confirm it clears the project's minimum-supported-runtime matrix.

Security

Ran a dedicated XSS pass over the new string-built HTML path. No XSS or injection issues identified. htmlLabelMarkup routes all attribute values through escapeAttr (sufficient for the double-quoted attrs used) and all label content through sanitizeText/renderKatexSanitized — the same sanitization the inline addHtmlSpan path already applied (the "double sanitize" is pre-existing and harmless). The canBatchSvgHtml() probe verifies namespace resolution and bails to per-element construction otherwise, so insertAdjacentHTML adds no mXSS surface, and the final DOMPurify.sanitize in mermaidAPI is untouched. The sanitizeText refactor preserves the exact DOMPurify config branches.


Really strong work — the hard part (making batched DOM caching correct) is well handled, and the perf payoff is real. The main ask before this leaves draft is nailing down that "no visual change" claim against those 8 repros; splitting out the parser fix would make both halves easier to land. Happy to re-review. 🙏

Severity tally: 🔴 0 · 🟡 3 · 💡 1 · 🟢 1 · 🎉 4 — draft PR, posted as COMMENT. Reviewed the full diff at head 501fb49; XSS pass clean; did not run the suites locally (working tree is on an unrelated branch).

Add a patch changeset for the batch node-label measurement/construction change
(the headline perf work had none; only the parser fix carried one).

Reword the render-regressions README so it is clear every fixture is a *fixed*
repro kept as a develop-parity check, not an outstanding regression.
@knsv-bot

Copy link
Copy Markdown
Collaborator Author

Thanks for the thorough pass — agreed it's non-blocking, but here's where each point landed.

Keeping the two perf fixes in one PR (intentionally). The label batching and the parser O(n²) fix came out of the same large-diagram profiling effort and are validated against the same Argos baseline; they're reviewed together and bisect together against that baseline. Splitting now would add rebase/CI churn without changing what a reviewer needs to read. They have separate changesets and separate tests, so they remain individually traceable within the one PR.

Changeset — added a patch changeset for the label-batching change (the parser fix already had one).

Parser suite — flowchart parser specs are green locally (962 passing; the lone skip is the intentional flow-huge case). The SPACE run-match only affects separator tokenization; the lone-\r edge you flagged isn't reachable as a meaningful token.

The 8 repros / "no visual change" — these are now fixed repros, not outstanding ones; reworded the README to say so. The Argos diff count tracked 120 → 17 → 0 across the fix commits, each round validated locally against develop. I'll confirm the final Argos build is fully green before taking it out of draft.

anyCommentRegex lookbehind — it is newly introduced. It's supported on Node 18+ and all evergreen browsers (Safari ≥ 16.4). If Mermaid's support floor predates that, the same guard has a lookbehind-free formulation and I'll switch to it — let me know the target matrix.

@knsv
knsv marked this pull request as ready for review June 18, 2026 11:18
@pbrolin47

Copy link
Copy Markdown
Collaborator

Hi @knsv-bot ,
Some things to address:

[sisyphos-bot]


What's Working Well

🎉 labelSignature() cache-invalidation — the JSON fingerprint of [label, width, isMarkdown, labelStyle, useHtmlLabels, centerLabel, icon, img] elegantly handles the "shape
mutates node after prebuild" edge case. Using it to detect stale cache entries at labelHelper call time is the right design.

🎉 canBatchSvgHtml() probe + structure-validation fallback — the one-time environment probe that tests whether insertAdjacentHTML assigns the correct SVG namespace, combined
with post-insertion child-count validation, is production-grade defensive coding. The fallback to per-element construction is seamless.

🎉 .raise() for paint order — a concise, correct fix. Appending a pre-built via insertAdjacentHTML deposits it at the end of the parent (correct order), then .raise() brings
it to the correct visual layer. Clean.

🎉 Regression fixture suite in cypress/platform/dev-diagrams/render-regressions/ — documenting the specific visual regressions (with Argos links) that this PR fixes is excellent
engineering. Future maintainers have a clear record of what to protect.

🎉 anyCommentRegex O(n²)→O(n) — the lookbehind (?:^|(?<=\S)) prevents catastrophic backtracking on deeply-indented diagrams. The accompanying regexes.spec.ts with an equivalence
oracle test and a regression guard for the perf case is exactly the right level of test coverage.

🎉 flowchart SPACE token — changing \s → [^\S\n\r]+ to match runs instead of single chars eliminates O(n·k) parse overhead on k leading spaces. Small but correct.


Things to Address

🟡 [important] — clearPrebuiltLabels() not in try/finally in createGraph.ts

packages/mermaid/src/rendering-util/createGraph.ts

The call sequence is:
await prebuildNodeLabels(nodesGroup, data4Layout.nodes.filter(...));
await Promise.all([insertNodes(...), insertEdges(...)]);
clearPrebuiltLabels();

If Promise.all rejects (e.g. a node shape renderer throws), clearPrebuiltLabels() never runs. This leaves stale entries in the prebuiltLabels Map and orphaned elements in
the SVG DOM. On the next render of the same diagram instance these orphans will be .raise()-d without re-measurement, and subsequent renders may see incorrect layout.

Suggested fix:
await prebuildNodeLabels(nodesGroup, filteredNodes);
try {
await Promise.all([insertNodes(...), insertEdges(...)]);
} finally {
clearPrebuiltLabels();
}


🟡 [important] — escapeAttr does not escape single quotes

packages/mermaid/src/rendering-util/createText.ts — escapeAttr function

export function escapeAttr(value: string): string {
return value
.replace(/&/g, '&')
.replace(/"/g, '"')
.replace(/</g, '<')
.replace(/>/g, '>');
}

All generated attributes use double-quote delimiters so a literal ' in a value doesn't break quoting today. However node.labelStyle is sourced from user-authored classDef
declarations and goes through this function into style="…" attributes. If any future change introduces single-quoted attribute delimiters, or if a style value is used in another
context, the missing escape becomes a live injection point.

' → ' is standard defence-in-depth for escapeAttr:
.replace(/'/g, ''')


🟢 [nit] — svgHtmlBatchSupport probe result cached module-globally forever

packages/mermaid/src/rendering-util/rendering-elements/shapes/util.ts

canBatchSvgHtml() caches its result in a module-level variable after the first call. This is intentional — it's a per-page-load environment capability test. However jsdom (used
by Vitest) returns false for this probe, which means unit tests always exercise the fallback path. That means buildNodeLabelsBatched gets zero unit-test coverage. Not a bug, but
worth a comment explaining why the batched path is validated via Cypress only.


Security

The XSS sub-agent found no confirmed XSS vectors introduced by this PR.

The batched path's security model is sound: attribute values go through escapeAttr; label body content (sanitizedLabel) is DOMPurify-sanitised twice (once at the
buildNodeLabelsBatched call site, once inside htmlLabelMarkup); insertAdjacentHTML targets an SVG (not document.body); and the overall output goes through a final
DOMPurify.sanitize() pass in mermaidAPI.ts.

The missing ' in escapeAttr is the only security-adjacent finding (logged above as 🟡). Pre-existing risk: user-supplied dompurifyConfig from %%{init}%% directives is passed
as-is to DOMPurify.sanitize() in sanitizeText(), which means a crafted dompurifyConfig: { ALLOWED_TAGS: ['script'] } could weaken the inner guard. This is pre-existing and not
introduced by this PR, but the new batched path raises the consequence of any such bypass — worth a follow-up audit.


Tests

  • Unit tests for labelSignature, escapeAttr, prebuildNodeLabels, and the regex equivalence oracle are present and appropriate.
  • buildNodeLabelsBatched is not covered by unit tests (jsdom probe always returns false). Cypress visual regression tests via the fixture suite provide coverage here.
  • Changesets present: batch-node-label-measurement.md (patch) and flowchart-deep-indent-parse-perf.md (patch) — both correct classifications.

knsv added 2 commits June 18, 2026 15:56
labelSignature gated on node.labelStyle, but every classDef-styled node sets its
label style (via styles2String) only after the prebuild pass — so the signature
mismatched and the node was discarded and rebuilt + re-measured inline. That
inline reflow-per-node was the dominant measure-phase cost on classDef-heavy
diagrams.

Build the prebuilt label with the node's final, classDef-derived label style up
front, and drop labelStyle from the signature: the prebuilt DOM and its measured
size are already correct, so colour/font need not be in the fingerprint. Styled
nodes now match and are reused — no inline rebuild, no per-node reflow — with
identical output.
configureLabelImages early-returns for label divs with no <img>, but the batched
prebuild still allocated a promise per label and awaited Promise.all over all of
them — adding an async suspension (idle wall-clock in the measure phase) even when
nothing needed loading. Filter to the labels that actually contain an image and
only await those; image-free diagrams (the common case) skip the suspension.

@knsv-bot knsv-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[sisyphus-bot]

This is excellent, careful performance work — the kind of shared-rendering-util change that's usually terrifying to review, made reviewable by how defensively it's built. I read the full diff at head (4b447b8) against develop, traced the batched-construction path for output-neutrality, ran an XSS pass over the new insertAdjacentHTML markup assembly, and validated the parser change against the full flowchart parser suite. The numbers are real and the safety net is genuinely good. A few things to discuss — one about scope, the rest small — but nothing blocking. Verdict: COMMENT.

What's working well

  • 🎉 It degrades to the exact pre-perf path at every layer. The canBatchSvgHtml() probe → per-element fallback, buildNodeLabelsBatched returning null on any structural mismatch, and labelHelper re-validating each prebuilt label via labelSignature before reuse (else rebuild inline) means correctness never depends on the fast path. This is exactly how to land a measure-phase rewrite safely.
  • 🎉 labelSignature is well-designed, and the reasoning for excluding labelStyle is the standout. The comment explaining that classDef colour doesn't affect text size, that the prebuild already stamps the final style, and that including it caused mass cache-misses on classDef-heavy diagrams — that's the detail that makes or breaks this optimization, and it's documented precisely.
  • 🎉 The fiddly correctness details are handled. .raise() to restore paint order for prebuilt labels (so erBox's hand-drawn background doesn't paint over the entity), configureLabelImages awaited only when a label actually contains an <img> (no idle await on the common case), and the updateNodeBounds 0×0 guard. Someone clearly hit each of these and fixed them deliberately.
  • 🎉 Output-neutrality is validated, not asserted. I checked htmlLabelMarkup against addHtmlSpan structurally (same foreignObject sizing, same div/span styles in the same order, labelStyle on both, same KaTeX branch); Argos is green ("approved") and all e2e pass. That's the right proof for a perf refactor.
  • 🎉 The parser change ships with an equivalence oracle. regexes.spec.ts proves the new anyCommentRegex strips byte-identically to the legacy pattern across a 20-case corpus, plus an O(n) perf assertion. The full flowchart parser suite (1006 tests) stays green under the flow.jison SPACE-run change.

Things to address

🟡 [important] Two unrelated perf changes in one PR — consider splitting

This bundles the label batching (rendering-util: createText.ts, util.ts, createGraph.ts, dagre/index.js, common.ts) with a flowchart parse-perf change (regexes.ts, flow.jison, regexes.spec.ts) — two separate concerns with two separate changesets. They touch entirely different subsystems and have very different risk profiles: the rendering-util batching is a high-regression-risk rewrite, while the parser change is small and self-contained. Bundling them means a revert of one reverts both, and git bisect can't separate a measure-phase regression from a parser regression. The parse-perf part is cleanly liftable into its own PR. Not blocking — just cleaner history and safer revertability if they're separate.

Nits & notes

  • 🟢 [nit] First lookbehind in the codebase. regexes.ts introduces (?<=\S) — a quick grep shows no other lookbehind anywhere in packages/mermaid/src. It's safe under the current effective targets (no explicit browserslist, so the default floor resolves to Safari/iOS 18+, well above the 16.4 where lookbehind landed). Worth a conscious note, though, because it sits in core preprocessing (anyCommentRegex runs for every diagram) and a lookbehind literal fails as a module-load SyntaxError on an unsupported engine — total breakage, not graceful degradation. If the team ever wants to keep the support floor low, an equivalent O(n) non-lookbehind pattern is achievable; if not, no action needed beyond awareness of the precedent.
  • 🟢 [nit] Prebuild/clear isn't exception-safe. In createGraphWithElements and measureDagreGraph, prebuildNodeLabels(...) → insert loop → clearPrebuiltLabels(...) aren't wrapped in try/finally. If the insert loop throws, entries linger in the module-global prebuiltLabels map (and measureDagreGraph's scoped clear never runs, so those leak permanently). In a long-lived page that re-renders on every keystroke (the live editor) with intermittently-invalid input, that can accumulate. A try { … } finally { clearPrebuiltLabels(scoped) } would close it.
  • 🟢 [nit] escapeAttr doesn't escape single quotes (createText.ts:126). Safe today since every attribute in the assembled markup is double-quoted, but a future single-quoted attribute would be exposed. A one-line comment ("double-quote-only") or adding ' to the set would harden it. (Surfaced by the XSS pass — see Security.)
  • 🟢 [nit] labelSignature width default differs from the build path — signature uses node.width || getConfig().flowchart?.wrappingWidth (can be undefined→'') while buildNodeLabelsBatched uses ?? 200. Worst case is a spurious signature miss → inline rebuild (safe, just a perf miss). Aligning the defaults would keep the cache-hit rate up.
  • 🟢 [note] The PR description carries a 🤖 Generated with Claude Code trailer. Per CLAUDE.md, AI-attribution trailers shouldn't appear in PR descriptions — worth trimming.

Security

Dedicated XSS/injection pass over the batched markup path — the obvious worry being raw label text reaching insertAdjacentHTML. No XSS regression. The string assembled for insertAdjacentHTML is built from DOMPurify-sanitized content at the same two sanitization layers as the old per-element path: the outer sanitizeText(decodeEntities(...)) in buildNodeLabelsBatched, and the inner hasKatex ? renderKatexSanitized : sanitizeText inside htmlLabelMarkup (mirroring addHtmlSpan's span.html(sanitizedLabel)). All attribute interpolations (getNodeClasses/cssClasses, domId/id, labelStyle) go through escapeAttr, every attribute is double-quoted, common.ts is a verbatim refactor (DOMPurify config byte-identical), and the terminal DOMPurify.sanitize() in mermaidAPI is untouched. The only finding is the single-quote nit above (not exploitable today).

Checks

  • ✅ Argos visual snapshots green ("3 changed — approved") — the output-neutrality proof
  • ✅ All e2e jobs pass; flowchart parser 1006 tests pass; regex equivalence + perf tests pass
  • ✅ ESLint 0 errors (11 pre-existing ||-vs-?? warnings in the large createText.ts), Prettier clean
  • ✅ deferMeasure defaults off, so every existing createText/addHtmlSpan caller is unchanged

Severity tally: 🔴 0 · 🟡 1 · 🟢 4 (+1 note) · 🎉 5. Genuinely impressive engineering — the safety architecture is what makes a change like this mergeable. The main ask is just deciding whether to split the parser change out; the rest is polish. 🚀

Reviewed the full diff at head 4b447b8 vs a freshly-fetched origin/develop, with structural parity checks on the batched-vs-inline label paths, the flowchart parser + regex suites run locally, and a dedicated XSS sub-review of the insertAdjacentHTML assembly.

- Wrap prebuild -> insert -> clear in try/finally in createGraphWithElements and
  measureDagreGraph so a throw mid-insert can't leak entries in the module-global
  prebuilt-label cache (matters for the long-lived live editor on invalid input).
- escapeAttr: also escape single quotes so the helper is safe for single-quoted
  attributes too (the assembled markup uses double quotes today).
- labelSignature: use the same width default (?? 200) as buildNodeLabelsBatched so
  the fingerprint matches the prebuilt label and avoids a spurious cache miss.
- regexes: note the anyCommentRegex lookbehind's minimum-runtime requirement.
knsv and others added 3 commits June 22, 2026 13:12
The flowchart parser O(n²) fix (anyCommentRegex guard, flow.jison SPACE-run
token, regexes.spec.ts equivalence test and its changeset) is an independent
concern from the label-batching work in this PR. Split out to #7906 per review
feedback so each can land and be bisected on its own. This PR is now
label-batching only.
scripts/editor.bash clones and builds the Mermaid Live Editor, which requires
Node >=24.16.0. Netlify selects the build's Node from this .node-version (the
file takes precedence over the NODE_VERSION env var), so the live editor's
pnpm install failed with ERR_PNPM_UNSUPPORTED_ENGINE on 22.14.0. Mermaid builds
and its full unit suite pass on 24.16.0, and Cypress 14.5.4 includes the Node
24.16.0 binary fix.
@aloisklink aloisklink self-assigned this Jul 1, 2026
knsv-bot pushed a commit that referenced this pull request Aug 7, 2026
The leading \s* with the global flag re-scanned every indent run from each
offset, costing ~250ms on a deeply-indented diagram inside detectType /
preprocess. Guard the run start with (?:^|(?<=\S)) so \s* is attempted once per
whitespace run; the match positions, and therefore the stripped output, are
byte-identical. Adds regexes.spec.ts: equivalence vs the original
/\s*%%.*\n/gm over a comment corpus plus an O(n) regression guard. Also landing
on #7872.
pull Bot pushed a commit to UltramarineSky/mermaid that referenced this pull request Sep 1, 2026
Two whitespace-related regex pathologies made deeply-indented flowcharts parse
in quadratic time:

- anyCommentRegex (used by diagram-type detection and preprocessing) re-scanned
  each indent run from every position because of its leading greedy \s*. Guarding
  where the run may start makes it linear, with byte-identical output (proven by
  regexes.spec.ts against the legacy pattern as an oracle, plus an O(n) assertion).
- The jison flowchart lexer matched a single whitespace char per SPACE token; it
  now matches a whitespace run. The grammar only uses SPACE as a separator, so
  parsing is unchanged.

A deeply-indented fixture that took ~1.4 s to parse now takes ~30 ms.

Split out of mermaid-js#7872 per review feedback so it can land and bisect independently.
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.

4 participants