Skip to content

Performance: selective incremental redraw + lighter full-render draw - #2079

Open
trvrb wants to merge 4 commits into
masterfrom
perf-implement
Open

Performance: selective incremental redraw + lighter full-render draw#2079
trvrb wants to merge 4 commits into
masterfrom
perf-implement

Conversation

@trvrb

@trvrb trvrb commented Jul 2, 2026

Copy link
Copy Markdown
Member

Via Claude

Summary

Application-code performance optimizations for the phyloTree renderer, targeting the two dominant costs on large trees: the incremental update path (phylotree.change()modifySVG, every date-filter/animation tick) and the full-render draw (drawBranches/drawTips, every load and layout change).

Stacked on #2078 (perf-infra), which adds the profiling harness (npm run profile) and the render-equivalence suite (npm run render-equiv) used to measure and guard every change here. Base is perf-infra; retarget to v3 once that merges.

All numbers below are on the 35k-tip spike-sm tree from https://nextstrain.org/groups/trajectories/spike-sm

Changes

1. Selective redraw on visibility change (change.ts) — phylotree.change() blanket-marked every node d.update = true on any visibility change, discarding the selective changed-set that updateNodesWithNewData() already computes from the visibility/stroke-width deltas. That forced modifySVG to rewrite the path + styles on all ~50k branches every tick. A visibility change does not move node positions (setDisplayOrder/setLayout are gated off for it; mapToScreen's domain keys on inView, which only zoom changes), so the selective set is sufficient.
→ per-tick modifySVG 383ms → 84ms (−78%), phylotree.change() 427ms → 126ms (−70%).

2. Flag stem-children on thickness change (change.ts) — A stem's start point is offset by its parent's stroke-width (xBase − 0.5·(parentStrokeWidth − strokeWidth)). The selective set flags a branch only when its own thickness changes, so a branch whose parent thickened but whose own thickness held kept a stale stem start (up to ~5px off; endpoint always correct). We now also flag children of every thickness-changed branch so their stem d is recomputed — making the selective redraw byte-identical to a full render.

3. Lighter full-render draw (renderers.ts, change.ts) — drawBranches/drawTips wrote constant presentation styles (pointer-events, cursor, stroke-linecap, tee fill:none, tip stroke-width) inline on each of ~67k elements. These are set only at draw time (modifySVG never touches them), so we set them once on each parent <g> and let SVG style-inheritance apply them — removing ~150k+ per-element style writes. Also removed the dead branch-gradient machinery (updateColorBy is a no-op; strokeForBranch returns a plain colour; gradients disabled since 2020) and inlined strokeForBranch in the stem stroke callback.
drawBranches −8–10% (~25–40ms), drawTips up to −8% per full render.

4. Batch A: O(N) cleanups (layouts.ts, colorHelpers.js) — Behaviour-preserving: collapse a chained .filter().filter() in mapToScreen to one pass; replace Math.max(...nodes.map()…) in radialLayout with a single loop (also removes a spread-argument stack-overflow risk on 35k-tip trees); fuse two chained .map() passes in calcNodeColor.

Why this is render-safe

By suite. npm run render-equiv drives each operation through the real incremental path in-app (history.pushState + a popstate event, which the app turns into a phylotree.change() — zero source changes) and asserts the settled SVG is byte-identical to a from-scratch full render of the same end state. Fresh run on this branch: 19/19 passed, 0 mismatches, covering colorBy (categorical + continuous), all four layouts, distance toggle, date filter, trait filter, zoom-to-clade, confidence, four multi-step sequences, plus zika and spike-sm (spike checks pass with 134k changed elements). For change (3) the snapshot reads getComputedStyle for the now-inherited constants, so it still verifies they resolve identically in both paths.

By logic.

  • (1) is safe because a visibility change provably does not move nodes — the position inputs (setDisplayOrder/setLayout/inView) are all gated off for it, so the pre-computed selective d.update set is exactly the set of elements whose DOM differs. The suite's datefilter, traitfilter, and their sequences exercise this.
  • (2) closes the one gap (1) opened (stale stem start on unchanged-thickness children); the suite's filter/date scenarios go byte-identical only with it in place — this fix is exactly what took them from 1 mismatch to 0.
  • (3) moves only constants that modifySVG never sets, so no incremental path can produce a different value; per-node props (cx/cy/r, fill, stroke, visibility, d, per-node stroke-width, visibility-dependent cursor) stay inline and unchanged. The removed gradient code was already inert.
  • (4) is pure computational refactor — same outputs, fewer array passes.

Test plan

  • npm run render-equiv — 19/19, 0 mismatches (incremental ≡ full render)
  • npm run profile before/after vs perf-infra baseline — deltas above
  • npm run type-check && npm run lint
  • Manual smoke on spike-sm: date-filter animation, zoom, layout switch, colorBy, confidence, hover/click

🤖 Generated with Claude Code

@trvrb
trvrb requested a review from jameshadfield July 2, 2026 02:02
Base automatically changed from perf-infra to v3 August 23, 2026 23:39
trvrb and others added 4 commits August 24, 2026 11:40
Low-risk, behaviour-preserving cleanups on the layout/color hot paths:
- mapToScreen: collapse a chained .filter().filter() over all nodes into a
  single-pass predicate.
- radialLayout: replace Math.max(...nodes.map().filter()) with a single loop.
  Besides avoiding two extra full-array passes, this removes a spread-argument
  stack-overflow risk on very large (35k-tip) trees.
- calcNodeColor: fuse two chained .map() passes over all nodes into one.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
phylotree.change() blanket-marked every node d.update=true on any visibility
change (date filter / animation tick), discarding the selective changed-set
that updateNodesWithNewData() computes from the visibility and stroke-width
deltas. This forced modifySVG to rewrite the path + styles on all ~50k branch
elements every tick.

A visibility change does not move node positions (setDisplayOrder/setLayout are
gated off for it, and mapToScreen's domain uses inView, which only zoom changes),
so the selective set is sufficient. Dropping changeVisibility from the blanket
flag lets modifySVG touch only the changed band.

On the 35k-tip spike-sm tree this cuts the per-tick modifySVG from ~383ms to
~84ms (-78%) and phylotree.change() from ~427ms to ~126ms (-70%); a headless
render-equivalence check confirms the incremental redraw is byte-identical to a
full render (0 mismatches across ~100k branch paths + ~35k tips).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
A branch stem's start point is offset by its stem-parent's stroke-width
(mapToScreen: xBase - 0.5*(parentStrokeWidth - strokeWidth)). The selective
redraw flags a branch only when its own stroke-width changes, so a branch whose
parent thickened but whose own thickness held would keep a stale stem start (up
to ~5px off, endpoint always correct). When branch thickness changes we now also
flag the children of every thickness-changed branch, so their stem `d` is
recomputed — making the incremental render byte-identical to a full render.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…adient code)

drawBranches/drawTips wrote several constant presentation styles (pointer-events,
cursor, stroke-linecap, tee fill:none, tip stroke-width) inline on every one of
~67k elements. These are set only at draw time (modifySVG never touches them), so
set them once on each parent <g> group and let SVG style-inheritance apply them —
removing ~150k+ per-element style writes from the draw. Also remove the dead
branch-gradient machinery (updateColorBy is a no-op and strokeForBranch returns a
plain colour; gradients disabled since 2020) and inline strokeForBranch in the
stem stroke callback.

On the 35k-tip spike-sm tree this cuts drawBranches ~8-10% (~25-40ms) and drawTips
up to ~8% per full render. Render-equivalence stays 19/19 (per-node props
unchanged) and a computed-style check confirms the group-inherited constants are
applied identically.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jameshadfield

Copy link
Copy Markdown
Member

Following on from #2077 (comment), my plan is to get this and #2077 merged in for a 3.1.0 release. The performance harness code #2078 is merged into v3 and will be part of the 3.0.0 release

Base automatically changed from v3 to master September 2, 2026 22:01
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.

2 participants