Skip to content

perf(inference): decouple legend/precision toggles from full chart rebuild#446

Open
Oseltamivir wants to merge 3 commits into
masterfrom
perf/decouple-toggle-from-chart-rebuild
Open

perf(inference): decouple legend/precision toggles from full chart rebuild#446
Oseltamivir wants to merge 3 commits into
masterfrom
perf/decouple-toggle-from-chart-rebuild

Conversation

@Oseltamivir

@Oseltamivir Oseltamivir commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Context

Main fix for the failing field INP (CrUX p75 273 ms, 15% of loads >500 ms). A Firefox profile of the live inference tab shows nearly every long task (220–490 ms) contains a click: each legend hw toggle / precision toggle / display switch tears down and rebuilds both charts — structure, axis tick text, grid, every dot re-joined and .raise()d, zoom re-attached and replayed.

docs/d3-charts.md ("Why 4 Effects") already prescribes the intended behavior: display toggles should be an opacity-only ~20 ms path. The D3Chart refactor lost that split. This PR restores it.

Why every toggle rebuilt

Three identity leaks each independently re-triggered useD3ChartRenderer's full render effect:

  1. layers memo had 23 deps incl. effectiveActiveHwTypes, selectedPrecisions, isPointVisible, getCssColor, resolveColor, activeOverlayHwTypes, knownIssueAnnotations
  2. x/yScaleConfig were fresh objects every render even when the computed domain was numerically identical
  3. the D3Chart data prop was the filtered point set (new identity per toggle)

Changes

  • interactionRef — layer closures (renders, zoom handlers, hover handlers) read visibility predicates, color resolvers, precisions, overlay sets, and known-issue annotations through a ref (the established refs-over-closures pattern from docs/pitfalls.md), so those values leave the layers/tooltipConfig dependency arrays.
  • useStableValue (new hook) — scale configs are compared by value; toggles that leave the domain untouched keep the previous object. Domain-changing toggles still rebuild and animate exactly as before ("Axis Domains from Visible Data Only" behavior is intact — covered by a dedicated test).
  • data={pointsData} — the unfiltered point set (visibility was already opacity-based by design; scales come from the scale configs).
  • Decoration layout effect — on toggle, restyles the existing DOM: dot opacity/pointer-events/fill/shape + tracked-ring color (hand-rolled pass that skips per-point label-text writes), then re-runs the rooflines and known-issues custom layer renders (stroke recolor, label placement, annotations) with zoom-aware scales.
  • scatter-points — shape-sync extracted to syncPointShape; optional getShapeKey accessor (existing selectedPrecisions config still supported).
  • processedOverlayData returns a stable empty array when no overlay is loaded, so precision toggles don't churn layers through it.

Deliberately preserved behaviors:

  • Recolor on toggle: dropping an hw redistributes the remaining vendor hues (dynamic-colors derives hues from the active set) — now applied by the decoration pass instead of a rebuild. High-contrast and theme switches also recolor without rebuilds.
  • Domain rescale on toggle with the existing data-update animation.
  • Shape-per-precision mapping, tracked rings, dblclick tracking, tooltip dismissal effects.

Impact

Non-domain-changing toggles drop from ~220–490 ms of main-thread blocking to a few ms of attribute writes (plus the React render). Domain-changing toggles keep the (now single-pass, see #445) rebuild. Together with #444/#445 this targets moving INP p75 well under the 200 ms "Good" threshold.

Overlay support (?unofficialrun=)

Works for both official runs and overlays: overlay roofline/line-label visibility flows through the same ref + decoration path; overlay X-markers and rooflines survive official toggles untouched. Covered by a dedicated integration test using mocked overlay context (2 overlay points + roofline + run labels). Please also verify on the preview deployment with a ?unofficialrun=<run-id> URL — I could not load a live overlay run locally.

GPUGraph (timeline mode) still uses the old path — it benefits from #444/#445; applying the same decoupling there is a follow-up.

Tests

New (all fail against the previous implementation where marked):

  • ScatterGraph.decoration.test.tsx — integration, real D3 in jsdom, rebuilds counted via a spy on setupChartStructure:
    • hw toggle hides via opacity without rebuild (regression)
    • active-set change recolors remaining series without rebuild (regression)
    • second precision swaps shapes (circle→square) without rebuild (regression)
    • domain-changing toggle still rebuilds (rescale behavior intact)
    • overlay markers/rooflines survive official toggles without rebuild (regression)
  • scatter-points.test.tsgetShapeKey accessor, in-place shape swap, selectedPrecisions fallback, syncPointShape create/update/swap
  • useStableValue.test.ts — identity preservation/adoption semantics

Full unit suite passes (106 files / 2048 tests in packages/app). pnpm typecheck / lint / fmt clean. next build compiles (page-data collection needs DATABASE_READONLY_URL, unavailable locally — CI covers it). Cypress integration specs need the DB-backed dev server; relying on CI for those.


Note

Medium Risk
Touches core inference chart rendering and D3 lifecycle; behavior is heavily tested but wrong decoration timing could cause visual glitches or missed rebuilds on domain changes.

Overview
Restores the cheap legend/precision toggle path for inference ScatterGraph so hardware, precision, and display filters update the existing SVG instead of triggering a full D3 teardown (~300ms main-thread work behind poor INP).

ScatterGraph routes visibility, colors, precisions, and annotations through interactionRef so those values leave the layers memo; useStableValue keeps x/y scale config referentially stable when domains are unchanged; D3Chart data is the unfiltered pointsData (visibility stays opacity-based). A new useLayoutEffect decoration pass restyles dots (opacity, fill, shape via syncPointShape), rooflines, and labels without rewriting animated transform / roofline d during pending data-update transitions. Full rebuilds still run when scale domains actually change.

scatter-points adds optional getShapeKey and shared syncPointShape for in-place shape swaps. useStableValue is a new hook; processedOverlayData returns a stable empty array when there is no overlay.

Tests: ScatterGraph.decoration.test.tsx (rebuild spy on setupChartStructure), useStableValue.test.ts, scatter-points.test.ts.

Reviewed by Cursor Bugbot for commit 880cd41. Bugbot is set up for automated code reviews on this repo. Configure here.

…build

Every legend hw toggle, precision toggle, optimal-only or high-contrast
switch tore down and rebuilt both inference charts: structure, axes
(tick text), grid, every dot re-joined and raised, zoom re-attached.
Profiling the live site shows each click costs 220-490ms of main-thread
blocking - the direct cause of the failing field INP (273ms p75,
15% of loads >500ms). The docs already prescribe the fix
(docs/d3-charts.md 'Why 4 Effects': display toggles should be an
opacity-only ~20ms path); the D3Chart refactor lost that split.

Three identity leaks forced the rebuild on every toggle:

- layers memo had 23 deps including effectiveActiveHwTypes,
  selectedPrecisions, isPointVisible, getCssColor, resolveColor,
  activeOverlayHwTypes, knownIssueAnnotations -> new layers array ->
  full render effect.
- x/yScaleConfig were new objects per render even when the computed
  domain was numerically identical.
- the D3Chart data prop was the *filtered* point set, changing identity
  on every toggle.

Now:

- Layer closures read visibility/colors/shapes through interactionRef
  (the established refs-over-closures pattern, docs/pitfalls.md), so
  those values leave the layers/tooltipConfig deps.
- Scale configs pass through useStableValue with a by-value comparator:
  toggles that keep the domain keep the object. Domain-changing toggles
  still rebuild and animate exactly as before (axes rescale per
  docs 'Axis Domains from Visible Data Only').
- data={pointsData} (all points; visibility was already opacity-based).
- A decoration layout effect restyles the existing DOM on toggle: dot
  opacity/pointer-events/fill/shape + tracked-ring color (hand-rolled,
  skipping per-point label text writes), and re-runs the rooflines and
  known-issues custom layer renders (stroke recolor, label placement,
  annotations) with zoom-aware scales.
- processedOverlayData returns a stable empty array when no overlay is
  loaded so precision toggles don't churn layers through it.
- scatter-points: shape-sync logic extracted to syncPointShape and an
  optional getShapeKey accessor added (selectedPrecisions config still
  supported for other callers).

Recoloring on toggle is preserved: dropping an hw redistributes the
remaining vendor hues (dynamic-colors), applied by the decoration pass.
High-contrast and theme switches also recolor without a rebuild now.

Overlay (?unofficialrun=) support: overlay roofline/label visibility
flows through the same ref + decoration path; overlay markers and
rooflines survive official toggles untouched (covered by a dedicated
integration test).
@Oseltamivir Oseltamivir requested a review from adibarra as a code owner June 12, 2026 04:53
@vercel

vercel Bot commented Jun 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
inferencemax-app Ready Ready Preview, Comment Jun 12, 2026 5:36am

Request Review

…oggles

On a toggle that changes the scale domain (e.g. selecting/deselecting the
SKU that owns the axis extremes), the full render restores each surviving
element to its old position and schedules old->new 'data-update'
transitions for both dots and roofline paths. The decoration effect could
run in the same commit and re-ran the rooflines layer render, which set
each path's final d before its scheduled transition started - so the
transition interpolated destination->destination: the pareto curve
teleported while the dots animated. On main both animated together.

The decoration pass now never writes the attributes the entrance
transitions animate:

- roofline visibility + solid-stroke recolor are direct opacity/stroke
  writes (never d); gradient strokes keep their url() reference since
  gradient stops use the fixed parallelism palette
- parallelism/line label visibility is applied via data attributes
  (mirrors handleLegendHoverEnd)
- the rooflines layer re-render (which rewrites d and re-places labels)
  only runs when labels are shown AND no 'data-update' transition is
  scheduled or running - detected via d3's per-node schedule store,
  because d3.active() only reports started transitions
- dots were already safe (the hand-rolled pass never touches transform)

A commit-scoped skip flag was tried first and rejected: the render effect
also runs in D3Chart-only commits (its dimensions state lives there),
where no ScatterGraph effect could reliably reset the flag, and commit
batching makes 'same task' detection environment-dependent. Writing only
non-animated attributes is robust regardless of effect/commit ordering.

Regression test: domain-changing toggle leaves the surviving roofline at
its old d and the surviving dot at its old transform at commit end, each
with a pending data-update transition (fails against the previous
decoration implementation, which leaves d at the destination).
@Oseltamivir

Copy link
Copy Markdown
Contributor Author

Fixed the animation regression reported in review: on a domain-changing toggle (new SKU selected/deselected), the pareto roofline teleported to its destination while the dots animated.

Cause: the decoration effect could run in the same commit as the full render, after the renderer had restored old positions and scheduled the old→new data-update transitions (they only start on the next timer tick). Decoration's rooflines layer re-render set each path's final d immediately, so the transition started at its destination — no-op for the curve, while the dots (whose transform decoration never touches) animated normally.

Fix (1f4b86c): the decoration pass now never writes the attributes entrance transitions animate:

  • roofline visibility/recolor → direct opacity/stroke writes (never d)
  • parallelism/line-label visibility → direct opacity writes via data attributes
  • the rooflines layer re-render (rewrites d + label placement) only runs when labels are shown and no data-update transition is scheduled/running on any roofline

A commit-scoped "skip if rebuilt in this commit" flag was tried first and rejected — the render effect also fires in D3Chart-only commits (dimensions state lives there), where ScatterGraph effects can't reliably reset such a flag; the new tests caught that variant leaving toggles undecorated after resize commits.

New regression test: animates rooflines together with dots on a domain-changing toggle — asserts the surviving roofline still sits at its old d (and the dot at its old transform) at commit end with a pending data-update transition. Fails against the previous decoration implementation with the exact teleported path as the received value.

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.

1 participant