perf(rendering): cut measure-phase reflows for node sizing#7871
Conversation
Two forced-reflow reductions in the per-node measure path, the dominant cost of rendering large diagrams: - labelHelper/insertLabel: for HTML labels the SVG <text> getBBox() result is immediately overwritten by the inner div's getBoundingClientRect (text is the oversized foreignObject). Skip that dead read on the HTML path. - drawRect: a plain axis-aligned <rect>'s getBBox equals the width/height we just drew, so pass those known bounds to updateNodeBounds instead of forcing a getBBox reflow. Hand-drawn (roughjs) rects still measure, since their paths overflow the nominal box. updateNodeBounds gains an optional knownBounds argument for callers that already know their exact rendered geometry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 9cf68a6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #7871 +/- ##
==========================================
- Coverage 2.91% 2.91% -0.01%
==========================================
Files 657 657
Lines 70417 70452 +35
Branches 979 979
==========================================
Hits 2055 2055
- Misses 68362 68397 +35
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
On large diagrams the dagre path emits thousands of per-node/edge/cluster log lines; whenever anything captures console output (the dev explorer, an app logger, DevTools) that log volume — not the layout — dominates render wall-clock. - mermaid-graphlib.js: downgrade ~20 misleveled `log.warn` algorithm traces to `log.debug` (they are internal diagnostics, not user warnings) - dagre/index.js: downgrade the per-render "Graph at first" `log.warn` to debug - edgeMarker.ts: stop warning for `none`/empty arrow type (valid "no arrowhead"); only genuinely unknown types warn - dev-explorer console panel: cheap manual `formatTs` (drop the `Intl` `toLocaleTimeString`), batch appends into one render per frame, and cap at 5000 entries — it was O(n²) on log volume (9s+ self-time in CPU profiles) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
knsv-bot
left a comment
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Nice, surgical perf work — and thank you for the profiler trace and the "why" comments, they made this fast to verify. Since it's a draft, treating this as early-stage direction feedback rather than a merge gate; nothing below is a blocker. I read the full diff at head 8d874c4, traced the three rendering changes for behavioral equivalence, and ran an XSS pass.
What's working well
- 🎉 The "no visual change" claim checks out on all three rendering changes. I verified each rather than taking it on faith:
updateNodeBounds(shapes/util.ts) — the existinggetBBox()path sets onlynode.width/node.height, and the newknownBoundsearly-return sets exactly those two and nothing else. No side effect is skipped.drawRect.ts:68-74— for a plain<rect>drawn with.attr('width', totalWidth).attr('height', totalHeight),getBBox()returns exactly those dimensions (stroke width andrx/rydon't affect the geometry bbox), so the analytic bounds are exact. Keeping hand-drawn/roughjs on the real-measurement path is the right call — those paths overflow the nominal box.- The HTML-label
getBBox()skip is a genuine dead read — the value was unconditionally overwritten bydiv.getBoundingClientRect()before any use, and label content still flows through the samesanitizeText/measurement path. edgeMarker.ts:80-85— for'none'/empty, the old code already fell into the!arrowTypeInfobranch and returned without writing any marker attribute, so the early return suppresses only alog.warnand an already-no-op path. Non-'none'arrow types are byte-identical.
- 🎉 The
knownBoundsJSDoc is exactly the warning a future caller needs — spelling out "only pass this when the value equals whatgetBBox()would return" is what keeps this from being misused on a shape where it isn't safe. - 🎉 The dev console-panel O(n²) fix is a tidy bit of work — coalescing per-entry
this.logs = [...]reassignments into one rAF-batched, bounded update is the right shape, andclear()correctly cancels the pending frame.
Things to consider (for when you take it out of draft)
🟡 The log-level downgrade and the real per-render cost are two different things
The warn → debug downgrades in mermaid-graphlib.js / dagre/index.js change console output, but the expensive part of several of those lines is the argument, which JS evaluates before the call regardless of level. At Mermaid's default logLevel: 'fatal', log.warn and log.debug are both no-op functions (logger.ts:41-72) — yet log.debug('Graph at first:', JSON.stringify(graphlibJson.write(graph))) still runs the full-graph serialization every render, and extractor does a graphlibJson.write(graph) per recursion. So:
- The downgrade is output-neutral at the default level (neither warn nor debug prints); it only changes what reaches the console for consumers at
warn/info(or the dev profiler capturing everything — which theconsole-panel.tscompanion fix addresses). - The CPU cost that would actually show up in a production large-diagram render — the eager
JSON.stringify(graphlibJson.write(graph))/graphlibJson.write(graph)argument expressions — is untouched by this PR and still runs at every level.
If the production-render win is the goal, the higher-leverage follow-up is to stop building those serialized strings (guard behind a level check, or only pass the raw graph and let the logger stringify lazily) rather than only changing the level. Not blocking, and the changeset is honest that this is about log volume — just flagging that the perf benefit here is mostly the dev-tooling path, not default renders. Also note dagre/index.js still has one log.warn('Graph after XAX:', JSON.stringify(graphlibJson.write(graph))) left at warn next to the one you downgraded — likely an oversight given the others.
💡 Changeset scope
The changeset (dagre-quiet-debug-logs.md) covers only the logging change. The forced-reflow reductions (util.ts / drawRect.ts) are the headline of the PR and are user-affecting (faster first paint), so they'd be worth their own changeset line — or, if this is intentionally landing as one entry for the whole perf series, a one-word note to that effect would save a future reader the double-take.
Housekeeping
- The PR description carries a
🤖 Generated with Claude Codetrailer, which the repo's own contribution policy asks us to keep out of PR descriptions — worth stripping before this leaves draft. mergeStateStatusis currentlyDIRTY(conflicts withdevelop) since it's stacked on #7870 — expected for the stack, just noting it'll need a rebase once #7870 lands so CI/Argos can run cleanly.
Security
Ran a dedicated XSS/injection pass. No XSS or injection issues identified. The substituted bounds are numeric layout sizes (never reach a DOM attribute as attacker-influenced strings), the edgeMarker early-return doesn't change any marker URL/attribute for valid arrow types, no new innerHTML/href/foreignObject/sink is introduced, DOMPurify config is untouched, and the dev console-panel binds log messages via Lit escaped text (no unsafeHTML).
Really clean change overall — the correctness reasoning is sound and the safety boundary (hand-drawn still measures) is exactly right. Looking forward to the batching PR that closes out the steady-state side. 🙏
Severity tally: 🔴 0 · 🟡 1 · 💡 1 · 🎉 3 — draft PR, so posted as COMMENT. Reviewed the full diff at head 8d874c4; XSS pass clean; did not run the suite (working tree is on an unrelated branch).
…-reflows # Conflicts: # packages/mermaid/src/rendering-util/rendering-elements/shapes/util.ts
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
Addresses review feedback on #7871: the warn→debug downgrade doesn't remove the real per-render cost, because the argument `JSON.stringify(graphlibJson.write(graph))` / `graphlibJson.write(graph)` serializes the whole graph and JS evaluates it on every render regardless of log level (both warn and debug are no-ops at the default `fatal` level). Comment those serializing logs out so the serialization never runs, and drop the now-unused `graphlibJson` import from both files. Also disables the leftover `log.warn('Graph after XAX', …)` the review flagged. Cheap per-element traces remain at `log.debug`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hi @knsv-bot Some minor things to address for this PR: What's working well 🎉 The knownBounds optimization in updateNodeBounds is the right abstraction. For axis-aligned plain rects, width and height ARE analytically exact at draw time — passing 🎉 Commit 3's insight is the most important correctness fix in this PR: JSON.stringify(graphlibJson.write(graph)) evaluates eagerly as a function argument, so JavaScript builds 🎉 The console-panel O(n²) fix is well-designed. Swapping synchronous per-entry DOM appends for RAF-batched #pending flushes is the right approach, and the 5000-entry cap 🎉 The edgeMarker.ts early return is a clean semantic fix — none IS a valid "no arrowhead" signal, not an unknown type, and silencing that log.warn removes misleading noise from Things to address 🟢 [nit] updateNodeBounds — knownBounds guard doesn't account for zero dimensions If knownBounds is { width: 0, height: 0 } (e.g. a degenerate zero-size rect), the function returns early and sets node.width = 0, node.height = 0. The getBBox() fallback would 🟢 [nit] dagre/mermaid-graphlib.js — one stray log.warn remains Tests The knownBounds fast path doesn't have a dedicated unit test, but that's acceptable here — correctness is verified by the full Cypress visual suite, and the path is simple Changeset dagre-quiet-debug-logs.md — patch bump — present and appropriately scoped. ✓ |
3 participants
Summary
Two targeted forced-reflow reductions in the per-node measure path. Layout-agnostic
(dagre + elk), no visual change. Part 2 of a large-diagram render-perf series
(profiler → this → batching). Stacked on #7870.
Evidence
A Chrome trace of a large dagre flowchart put measure at 461 ms / 55% of render,
of which instrumented
Layout(forced reflow) was 173 ms across 2,216 events —~2 forced reflows per node (write DOM → read size → write → read …).
Changes
getBBoxfor HTML labels (shapes/util.ts): for HTML labelsthe SVG
<text>getBBox()result is immediately overwritten by the inner div'sgetBoundingClientRect()(andtextis the oversized foreignObject), so it's adead read. Measure
getBBoxonly on the non-HTML path.drawRect.ts+updateNodeBounds): a plain<rect>'sgetBBox()equals the width/height just drawn, so pass those knownbounds instead of forcing a reflow. Hand-drawn (roughjs) rects still measure.
Where it helps
These remove forced reflows that dominate the cold first render (page load). In
the warm re-render benchmark forced reflow is only ~4% of measure, so the win here
is first-paint latency; the steady-state win is in the follow-up batching PR.
Testing
Unit tests pass; cypress visual snapshots clean (rendered SVG unchanged).
🤖 Generated with Claude Code