From e27fd0d1cad0e5611a0a9409c454eaf13bf3e717 Mon Sep 17 00:00:00 2001 From: Oseltamivir <58582368+Oseltamivir@users.noreply.github.com> Date: Thu, 11 Jun 2026 21:52:25 -0700 Subject: [PATCH 1/2] perf(inference): decouple legend/precision toggles from full chart rebuild 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). --- .../ui/ScatterGraph.decoration.test.tsx | 325 ++++++++++++++++++ .../components/inference/ui/ScatterGraph.tsx | 244 ++++++++++--- packages/app/src/hooks/useStableValue.test.ts | 104 ++++++ packages/app/src/hooks/useStableValue.ts | 21 ++ .../d3-chart/layers/scatter-points.test.ts | 165 +++++++++ .../src/lib/d3-chart/layers/scatter-points.ts | 80 +++-- 6 files changed, 862 insertions(+), 77 deletions(-) create mode 100644 packages/app/src/components/inference/ui/ScatterGraph.decoration.test.tsx create mode 100644 packages/app/src/hooks/useStableValue.test.ts create mode 100644 packages/app/src/hooks/useStableValue.ts create mode 100644 packages/app/src/lib/d3-chart/layers/scatter-points.test.ts diff --git a/packages/app/src/components/inference/ui/ScatterGraph.decoration.test.tsx b/packages/app/src/components/inference/ui/ScatterGraph.decoration.test.tsx new file mode 100644 index 00000000..097355fb --- /dev/null +++ b/packages/app/src/components/inference/ui/ScatterGraph.decoration.test.tsx @@ -0,0 +1,325 @@ +// @vitest-environment jsdom +/** + * Integration tests for the ScatterGraph toggle decoration path. + * + * Core behavior under test: legend hw toggles, precision toggles, and color + * changes restyle the existing SVG (opacity / fill / shape) WITHOUT tearing + * down and rebuilding the chart structure — the ~300ms main-thread long task + * behind the failing field INP. A rebuild is detected via a spy on + * setupChartStructure, which every full chart render must call. + */ +import { act, createElement, useReducer } from 'react'; +import { createRoot } from 'react-dom/client'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { setupChartStructure } from '@/lib/d3-chart/chart-setup'; +import type { ChartDefinition, InferenceData } from '@/components/inference/types'; + +// ── Module mocks ─────────────────────────────────────────────────────────── +vi.mock('@/lib/d3-chart/chart-setup', { spy: true }); +vi.mock('@/lib/analytics', () => ({ track: vi.fn() })); +vi.mock('next-themes', () => ({ useTheme: () => ({ resolvedTheme: 'dark' }) })); +// The legend is React-rendered (covered elsewhere) — keep the tree light. +vi.mock('@/components/ui/chart-legend', () => ({ default: () => null })); + +const inferenceState = vi.hoisted(() => ({ current: {} as Record })); +vi.mock('@/components/inference/InferenceContext', () => ({ + useInference: () => inferenceState.current, +})); + +const overlayState = vi.hoisted(() => ({ current: {} as Record })); +vi.mock('@/components/unofficial-run-provider', () => ({ + useUnofficialRun: () => overlayState.current, +})); + +import ScatterGraph from './ScatterGraph'; + +// ── Environment stubs ──────────────────────────────────────────────────────── +class MockResizeObserver { + observe() {} + unobserve() {} + disconnect() {} +} + +// ── Fixtures ───────────────────────────────────────────────────────────────── +const point = (hwKey: string, precision: string, x: number, y: number, tp: number): InferenceData => + ({ hwKey, precision, x, y, tp, conc: 16, framework: 'vllm' }) as unknown as InferenceData; + +// h100 owns both axis extremes so hiding b200 / showing fp4 keeps the niced +// domains identical — exactly the toggle case that must not rebuild. +const POINTS: InferenceData[] = [ + point('h100', 'fp8', 1, 1, 1), + point('h100', 'fp8', 100, 1000, 8), + point('h100', 'fp4', 40, 400, 4), + point('b200', 'fp8', 50, 500, 4), + point('b200', 'fp8', 60, 600, 8), +]; + +const HARDWARE_CONFIG = { + h100: { name: 'H100', label: 'H100', gpu: 'H100' }, + b200: { name: 'B200', label: 'B200', gpu: 'B200' }, +}; + +const CHART_DEFINITION = { chartType: 'interactivity' } as unknown as ChartDefinition; + +const noop = () => {}; + +function baseInferenceState() { + return { + activeHwTypes: new Set(['h100', 'b200']), + hardwareConfig: HARDWARE_CONFIG, + toggleHwType: noop, + removeHwType: noop, + hwTypesWithData: new Set(['h100', 'b200']), + selectedPrecisions: ['fp8'], + selectedYAxisMetric: 'y', + availableRuns: null, + selectedRunId: '', + hideNonOptimal: false, + setHideNonOptimal: noop, + hidePointLabels: false, + setHidePointLabels: noop, + selectAllHwTypes: noop, + highContrast: false, + setHighContrast: noop, + logScale: false, + setLogScale: noop, + scaleType: 'auto', + isLegendExpanded: false, + setIsLegendExpanded: noop, + useAdvancedLabels: false, + setUseAdvancedLabels: noop, + showGradientLabels: false, + setShowGradientLabels: noop, + showLineLabels: false, + setShowLineLabels: noop, + showSpeedOverlay: false, + setShowSpeedOverlay: noop, + showMinecraftOverlay: false, + setShowMinecraftOverlay: noop, + trackedConfigs: [], + addTrackedConfig: noop, + removeTrackedConfig: noop, + }; +} + +function baseOverlayState() { + return { + isUnofficialRun: false, + activeOverlayHwTypes: new Set(), + setActiveOverlayHwTypes: noop, + allOverlayHwTypes: new Set(), + toggleOverlayHwType: noop, + resetOverlayHwTypes: noop, + localOfficialOverride: null, + setLocalOfficialOverride: noop, + runIndexByUrl: {}, + unofficialRunInfos: [], + }; +} + +// ── Harness ────────────────────────────────────────────────────────────────── +function mountChart(props?: Partial[0]>) { + let forceUpdate: () => void = noop; + function Harness() { + // ScatterGraph and D3Chart are React.memo'd; mocked context hooks bypass + // React's context subscription, so re-renders are driven through a + // version-bumped caption prop. + const [version, bump] = useReducer((v: number) => v + 1, 0); + forceUpdate = bump; + return createElement(ScatterGraph, { + chartId: 'chart-test', + modelLabel: 'DeepSeek-R1-0528', + data: POINTS, + xLabel: 'Interactivity (tok/s/user)', + yLabel: 'Output Throughput per GPU', + chartDefinition: CHART_DEFINITION, + transitionDuration: 0, + caption: `v${version}`, + ...props, + }); + } + + const container = document.createElement('div'); + document.body.append(container); + const root = createRoot(container); + act(() => { + root.render(createElement(Harness)); + }); + return { + container, + rerender: () => act(() => forceUpdate()), + unmount: () => { + act(() => root.unmount()); + container.remove(); + }, + }; +} + +const dotGroups = (container: HTMLElement, hwKey?: string) => + [...container.querySelectorAll('.dot-group')].filter( + (n) => !hwKey || n.dataset.hwKey === hwKey, + ); + +const rebuildCount = () => vi.mocked(setupChartStructure).mock.calls.length; + +beforeEach(() => { + vi.stubGlobal('ResizeObserver', MockResizeObserver); + // Charts measure their container; jsdom reports 0 — give them real space. + vi.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockReturnValue({ + width: 800, + height: 600, + top: 0, + left: 0, + right: 800, + bottom: 600, + x: 0, + y: 0, + toJSON: () => ({}), + } as DOMRect); + inferenceState.current = baseInferenceState(); + overlayState.current = baseOverlayState(); + vi.mocked(setupChartStructure).mockClear(); +}); + +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +describe('ScatterGraph toggle decoration', () => { + it('renders all points and rooflines after mount', () => { + const { container, unmount } = mountChart(); + + expect(dotGroups(container)).toHaveLength(POINTS.length); + expect(container.querySelectorAll('.roofline-path').length).toBeGreaterThan(0); + expect(rebuildCount()).toBeGreaterThan(0); + unmount(); + }); + + it('hides a toggled-off hw via opacity without rebuilding the chart', () => { + const { container, rerender, unmount } = mountChart(); + const buildsAfterMount = rebuildCount(); + + inferenceState.current = { + ...inferenceState.current, + activeHwTypes: new Set(['h100']), + }; + rerender(); + + for (const dot of dotGroups(container, 'b200')) { + expect(dot.style.opacity).toBe('0'); + expect(dot.style.pointerEvents).toBe('none'); + } + for (const dot of dotGroups(container, 'h100').filter((d) => d.dataset.precision === 'fp8')) { + expect(dot.style.opacity).toBe('1'); + expect(dot.style.pointerEvents).toBe('auto'); + } + const b200Roofline = container.querySelector( + '.roofline-path[data-hw-key="b200"]', + ); + expect(b200Roofline).not.toBeNull(); + expect(b200Roofline!.style.opacity).toBe('0'); + + // The whole point: a legend toggle is a restyle, not a teardown/rebuild. + expect(rebuildCount()).toBe(buildsAfterMount); + unmount(); + }); + + it('recolors remaining series when the active set changes, without rebuilding', () => { + const { container, rerender, unmount } = mountChart(); + const buildsAfterMount = rebuildCount(); + const h100Fill = () => + dotGroups(container, 'h100')[0].querySelector('.visible-shape')!.getAttribute('fill'); + const before = h100Fill(); + + // h100 and b200 share the NVIDIA hue zone: dropping one redistributes + // the remaining hues (dynamic-colors), so the dots must actually recolor. + inferenceState.current = { + ...inferenceState.current, + activeHwTypes: new Set(['h100']), + }; + rerender(); + + expect(h100Fill()).not.toBe(before); + expect(rebuildCount()).toBe(buildsAfterMount); + unmount(); + }); + + it('swaps point shapes when a second precision is selected, without rebuilding', () => { + const { container, rerender, unmount } = mountChart(); + const buildsAfterMount = rebuildCount(); + const fp4Dot = () => dotGroups(container, 'h100').find((d) => d.dataset.precision === 'fp4')!; + + // Single precision: fp4 points are hidden circles. + expect(fp4Dot().style.opacity).toBe('0'); + expect(fp4Dot().querySelector('.visible-shape')!.tagName.toLowerCase()).toBe('circle'); + + inferenceState.current = { + ...inferenceState.current, + selectedPrecisions: ['fp8', 'fp4'], + }; + rerender(); + + // Second precision becomes visible as the square shape (slot 2). + expect(fp4Dot().style.opacity).toBe('1'); + const shape = fp4Dot().querySelector('.visible-shape')!; + expect(shape.tagName.toLowerCase()).toBe('rect'); + expect(shape.dataset.shapeKey).toBe('square'); + expect(rebuildCount()).toBe(buildsAfterMount); + unmount(); + }); + + it('rebuilds when the scale domain actually changes (data refresh path intact)', () => { + const { rerender, unmount } = mountChart(); + const buildsAfterMount = rebuildCount(); + + // Hiding the hw that owns the axis extremes changes the visible domain — + // axes must rescale, which is a legitimate full render. + inferenceState.current = { + ...inferenceState.current, + activeHwTypes: new Set(['b200']), + }; + rerender(); + + expect(rebuildCount()).toBeGreaterThan(buildsAfterMount); + unmount(); + }); + + it('keeps unofficial-run overlay markers rendered through official toggles', () => { + const overlayPoints = [point('h100', 'fp8', 30, 300, 2), point('h100', 'fp8', 35, 350, 4)].map( + (p) => ({ ...p, run_url: 'https://github.com/o/r/actions/runs/123' }), + ); + overlayState.current = { + ...baseOverlayState(), + isUnofficialRun: true, + activeOverlayHwTypes: new Set(['h100']), + allOverlayHwTypes: new Set(['h100']), + runIndexByUrl: { 'https://github.com/o/r/actions/runs/123': 0 }, + unofficialRunInfos: [ + { id: '123', branch: 'test-branch', url: 'https://github.com/o/r/actions/runs/123' }, + ], + }; + const { container, rerender, unmount } = mountChart({ + overlayData: { + data: overlayPoints, + hardwareConfig: HARDWARE_CONFIG, + } as unknown as Parameters[0]['overlayData'], + }); + const buildsAfterMount = rebuildCount(); + + expect(container.querySelectorAll('.unofficial-overlay-pt')).toHaveLength(2); + expect(container.querySelectorAll('.overlay-roofline-path').length).toBeGreaterThan(0); + + // Toggling an official hw must not rebuild or disturb overlay markers. + inferenceState.current = { + ...inferenceState.current, + activeHwTypes: new Set(['h100']), + }; + rerender(); + + expect(container.querySelectorAll('.unofficial-overlay-pt')).toHaveLength(2); + expect(rebuildCount()).toBe(buildsAfterMount); + unmount(); + }); +}); diff --git a/packages/app/src/components/inference/ui/ScatterGraph.tsx b/packages/app/src/components/inference/ui/ScatterGraph.tsx index fa383ada..266a2ca0 100644 --- a/packages/app/src/components/inference/ui/ScatterGraph.tsx +++ b/packages/app/src/components/inference/ui/ScatterGraph.tsx @@ -2,7 +2,7 @@ import { track } from '@/lib/analytics'; import * as d3 from 'd3'; -import React, { useCallback, useEffect, useMemo, useRef } from 'react'; +import React, { useCallback, useEffect, useLayoutEffect, useMemo, useRef } from 'react'; import { GRADIENT_NUDGE_EVENT } from '@/lib/nudges/registry'; import { useInference } from '@/components/inference/InferenceContext'; @@ -22,7 +22,8 @@ import type { ZoomContext, } from '@/lib/d3-chart/D3Chart/types'; import type { ContinuousScale } from '@/lib/d3-chart/types'; -import { computeTooltipPosition } from '@/lib/d3-chart/layers/scatter-points'; +import { computeTooltipPosition, syncPointShape } from '@/lib/d3-chart/layers/scatter-points'; +import { useStableValue } from '@/hooks/useStableValue'; import { overlayRooflineDasharray, overlayRunColor, @@ -101,6 +102,27 @@ const formatChangelogDescription = (desc: string | string[]): React.JSX.Element const CHART_MARGIN = { top: 24, right: 10, bottom: 60, left: 60 }; +// Referentially stable "no overlay data" result (see processedOverlayData). +const EMPTY_OVERLAY_DATA: InferenceData[] = []; + +// Scale configs are recomputed from the visible points on every render, but a +// legend / precision toggle usually leaves the actual domain untouched (x-min +// is pinned at 0; extremes are owned by a handful of points). Comparing by +// value lets those toggles keep the previous config object, so the chart +// render effect doesn't tear down and rebuild the SVG for identical scales. +interface ScaleConfigValue { + type: 'log' | 'linear'; + domain: [number, number]; + nice: boolean; + _isLog?: boolean; +} +const isSameScaleConfig = (a: ScaleConfigValue, b: ScaleConfigValue): boolean => + a.type === b.type && + a.nice === b.nice && + a._isLog === b._isLog && + a.domain[0] === b.domain[0] && + a.domain[1] === b.domain[1]; + // Derive a readable label from a hwKey using the HARDWARE_CONFIG source of truth const parseHwKeyToLabel = (hwKey: string): { name: string; label: string } => { const config = getHardwareConfig(hwKey); @@ -343,7 +365,10 @@ const ScatterGraph = React.memo( ); const processedOverlayData = useMemo(() => { - if (!overlayData?.data) return []; + // Stable empty reference: without an overlay this must not churn on + // precision changes — it feeds the `layers` memo, and a new identity + // there forces a full chart rebuild. + if (!overlayData?.data) return EMPTY_OVERLAY_DATA; return overlayData.data.filter((p) => selectedPrecisions.includes(p.precision)); }, [overlayData, selectedPrecisions]); @@ -376,12 +401,6 @@ const ScatterGraph = React.memo( getCssColor, ]); - // Combined data for D3 scale domain (includes overlay so scales fit both datasets) - const chartScaleData = useMemo(() => { - if (processedOverlayData.length === 0) return filteredData; - return [...filteredData, ...processedOverlayData]; - }, [filteredData, processedOverlayData]); - const overlayRooflines = useMemo(() => { interface Entry { hwKey: string; @@ -473,7 +492,7 @@ const ScatterGraph = React.memo( const isInputTputMetric = selectedYAxisMetric === 'y_inputTputPerGpu'; - const xScaleConfig = useMemo(() => { + const xScaleConfigRaw = useMemo(() => { const ext = visiblePoints.length > 0 ? (d3.extent(visiblePoints, (d) => d.x) as [number, number]) @@ -497,8 +516,9 @@ const ScatterGraph = React.memo( _isLog: useLog, }; }, [visiblePoints, isInputTputMetric, xLabel, scaleType, niceAxes]); + const xScaleConfig = useStableValue(xScaleConfigRaw, isSameScaleConfig); - const yScaleConfig = useMemo(() => { + const yScaleConfigRaw = useMemo(() => { const ext = visiblePoints.length > 0 ? (d3.extent(visiblePoints, (d) => d.y) as [number, number]) @@ -521,6 +541,7 @@ const ScatterGraph = React.memo( nice: niceAxes, }; }, [visiblePoints, isInputTputMetric, logScale, niceAxes]); + const yScaleConfig = useStableValue(yScaleConfigRaw, isSameScaleConfig); // --- Axis configs --- const xAxisConfig = useMemo( @@ -566,6 +587,39 @@ const ScatterGraph = React.memo( [effectiveActiveHwTypes, selectedPrecisions], ); + // --- Interaction state ref --- + // Latest visibility predicates, color resolvers, and active sets — read by + // long-lived D3 closures (layer renders, zoom handlers, hover handlers). + // Routing these reads through a ref keeps them out of the `layers` / + // `tooltipConfig` dependency arrays, so a legend or precision toggle no + // longer tears down and rebuilds the whole chart: the decoration effect + // below restyles the existing DOM instead (see docs/d3-charts.md "Why 4 + // Effects" — this is the cheap Effect-4 "display toggle" path). Same + // refs-over-closures rule as docs/pitfalls.md "Stale Closures in D3 Event + // Handlers". + const interactionRef = useRef({ + isPointVisible, + effectiveActiveHwTypes, + selectedPrecisions, + activeOverlayHwTypes, + getCssColor, + resolveColor, + knownIssueAnnotations, + }); + interactionRef.current = { + isPointVisible, + effectiveActiveHwTypes, + selectedPrecisions, + activeOverlayHwTypes, + getCssColor, + resolveColor, + knownIssueAnnotations, + }; + + // Render context from the last D3 render — lets the decoration effect + // restyle with the same layout/scales the chart was drawn with. + const lastRenderCtxRef = useRef(null); + const handleLegendHover = useCallback( (hwKey: string) => { const svg = chartRef.current?.getSvgElement?.(); @@ -694,12 +748,12 @@ const ScatterGraph = React.memo( onHoverStart: (sel: d3.Selection, d: InferenceData) => applyHoverState( sel.select('.visible-shape') as any, - getShapeKeyForPrecision(d.precision, selectedPrecisions), + getShapeKeyForPrecision(d.precision, interactionRef.current.selectedPrecisions), ), onHoverEnd: (sel: d3.Selection, d: InferenceData) => applyNormalState( sel.select('.visible-shape') as any, - getShapeKeyForPrecision(d.precision, selectedPrecisions), + getShapeKeyForPrecision(d.precision, interactionRef.current.selectedPrecisions), ), onPointClick: (d: InferenceData) => { track('latency_data_point_clicked', { hw: String(d.hwKey), x: d.x, y: d.y }); @@ -736,7 +790,6 @@ const ScatterGraph = React.memo( addTrackedConfig, removeTrackedConfig, chartDefinition.chartType, - selectedPrecisions, ], ); @@ -747,6 +800,10 @@ const ScatterGraph = React.memo( type: 'custom', key: 'rooflines', render: (zoomGroup, ctx) => { + // Visibility / colors come from the interaction ref so this closure + // stays correct between layer recreations (toggles restyle via the + // decoration effect instead of rebuilding the chart). + const ir = interactionRef.current; const xScale = ctx.xScale as ContinuousScale; const yScale = ctx.yScale as ContinuousScale; const { defs } = ctx.layout; @@ -786,8 +843,8 @@ const ScatterGraph = React.memo( const hw = key.split('_').slice(0, -1).join('_'); const precision = key.split('_').pop()!; const visible = - effectiveActiveHwTypes.has(hw) && selectedPrecisions.includes(precision); - let stroke = getCssColor(resolveColor(hw)); + ir.effectiveActiveHwTypes.has(hw) && ir.selectedPrecisions.includes(precision); + let stroke = ir.getCssColor(ir.resolveColor(hw)); if (showGradientLabels) { const pointLabels = allPointLabelsByKey[key]; @@ -860,7 +917,7 @@ const ScatterGraph = React.memo( const hw = key.split('_').slice(0, -1).join('_'); const precision = key.split('_').pop()!; const visible = - effectiveActiveHwTypes.has(hw) && selectedPrecisions.includes(precision); + ir.effectiveActiveHwTypes.has(hw) && ir.selectedPrecisions.includes(precision); const segments: { label: string; color: string; points: InferenceData[] }[] = []; let cur = { @@ -960,7 +1017,7 @@ const ScatterGraph = React.memo( const isInteractivity = chartDefinition.chartType === 'interactivity'; // With >1 precision selected each precision is its own curve, so label // every curve and include the precision in the text. - const multiPrecision = selectedPrecisions.length > 1; + const multiPrecision = ir.selectedPrecisions.length > 1; const LABEL_H = 18; const LABEL_W = 120; // approximate label width for overlap check @@ -1008,7 +1065,7 @@ const ScatterGraph = React.memo( key: entry.key, hw: entry.hw, label, - color: getCssColor(resolveColor(entry.hw)), + color: ir.getCssColor(ir.resolveColor(entry.hw)), x: px, y: py, visible: true, @@ -1025,7 +1082,7 @@ const ScatterGraph = React.memo( key: entry.key, hw: entry.hw, label, - color: getCssColor(resolveColor(entry.hw)), + color: ir.getCssColor(ir.resolveColor(entry.hw)), x: xScale(pt.x), y: yScale(pt.y), visible: false, @@ -1042,7 +1099,7 @@ const ScatterGraph = React.memo( key: entry.key, hw: entry.hw, label: lineLabelText(entry.hw, entry.precision, multiPrecision), - color: getCssColor(resolveColor(entry.hw)), + color: ir.getCssColor(ir.resolveColor(entry.hw)), x: xScale(entry.points[0].x), y: yScale(entry.points[0].y), visible: false, @@ -1070,7 +1127,8 @@ const ScatterGraph = React.memo( }; const sortedOverlay = Object.entries(overlayRooflines) .filter( - ([, group]) => activeOverlayHwTypes.has(group.hwKey) && group.points.length >= 2, + ([, group]) => + ir.activeOverlayHwTypes.has(group.hwKey) && group.points.length >= 2, ) .toSorted(([, a], [, b]) => yScale(a.points[0].y) - yScale(b.points[0].y)); @@ -1134,7 +1192,7 @@ const ScatterGraph = React.memo( key: entry.key, hw: entry.hw, label: lineLabelText(entry.hw, entry.precision, multiPrecision), - color: getCssColor(resolveColor(entry.hw)), + color: ir.getCssColor(ir.resolveColor(entry.hw)), x: xScale(pt.x), y: yScale(pt.y), visible: true, @@ -1143,7 +1201,7 @@ const ScatterGraph = React.memo( // Endpoint labels for overlay rooflines too (one per (hw, runIndex)), // labeled with the run's branch name to mirror the overlay legend. for (const [ovKey, group] of Object.entries(overlayRooflines)) { - if (group.points.length < 2 || !activeOverlayHwTypes.has(group.hwKey)) continue; + if (group.points.length < 2 || !ir.activeOverlayHwTypes.has(group.hwKey)) continue; const info = unofficialRunInfos[group.runIndex]; const branchOrHw = info ? `✕ ${info.branch || `run ${info.id}`}` @@ -1232,6 +1290,7 @@ const ScatterGraph = React.memo( }); }, onZoom: (zoomGroup, ctx) => { + const ir = interactionRef.current; const newXScale = ctx.newXScale as ContinuousScale; const newYScale = ctx.newYScale as ContinuousScale; const { defs } = ctx.layout; @@ -1302,7 +1361,7 @@ const ScatterGraph = React.memo( // Update line label positions on zoom if (showLineLabels) { const isInteractivity = chartDefinition.chartType === 'interactivity'; - const multiPrecision = selectedPrecisions.length > 1; + const multiPrecision = ir.selectedPrecisions.length > 1; const LABEL_H = 18; const LABEL_W = 120; @@ -1319,7 +1378,8 @@ const ScatterGraph = React.memo( if (pts.length < 2) continue; const hw = key.split('_').slice(0, -1).join('_'); const prec = key.split('_').pop()!; - if (!effectiveActiveHwTypes.has(hw) || !selectedPrecisions.includes(prec)) continue; + if (!ir.effectiveActiveHwTypes.has(hw) || !ir.selectedPrecisions.includes(prec)) + continue; const groupKey = multiPrecision ? key : hw; const prev = bestByGroup.get(groupKey); if (!prev || pts.length > prev[1].length) bestByGroup.set(groupKey, [key, pts]); @@ -1361,7 +1421,8 @@ const ScatterGraph = React.memo( // official labels post-zoom. const overlayVisible = Object.entries(overlayRooflines) .filter( - ([, group]) => activeOverlayHwTypes.has(group.hwKey) && group.points.length >= 2, + ([, group]) => + ir.activeOverlayHwTypes.has(group.hwKey) && group.points.length >= 2, ) .toSorted(([, a], [, b]) => newYScale(a.points[0].y) - newYScale(b.points[0].y)); for (const [ovKey, group] of overlayVisible) { @@ -1417,7 +1478,8 @@ const ScatterGraph = React.memo( if (pts.length < 2) return; const hw = key.split('_').slice(0, -1).join('_'); const prec = key.split('_').pop()!; - if (!effectiveActiveHwTypes.has(hw) || !selectedPrecisions.includes(prec)) return; + if (!ir.effectiveActiveHwTypes.has(hw) || !ir.selectedPrecisions.includes(prec)) + return; const groupKey = multiPrecision ? key : hw; if (seen.has(groupKey)) return; seen.add(groupKey); @@ -1426,7 +1488,7 @@ const ScatterGraph = React.memo( }); // Overlay rooflines: per-(hw, runIndex) endpoint labels. for (const [ovKey, group] of Object.entries(overlayRooflines)) { - if (group.points.length < 2 || !activeOverlayHwTypes.has(group.hwKey)) continue; + if (group.points.length < 2 || !ir.activeOverlayHwTypes.has(group.hwKey)) continue; const pt = group.points.at(-1)!; zoomLabels.push({ key: `overlay-${ovKey}`, @@ -1472,11 +1534,16 @@ const ScatterGraph = React.memo( key: 'points', data: pointsData, config: { + // Visibility / colors / shapes read the interaction ref so these + // accessors stay current between layer recreations (toggles restyle + // via the decoration effect instead of rebuilding the chart). getColor: (d) => (showGradientLabels && gradientColorByPoint.get(d)) || - getCssColor(resolveColor(d.hwKey as string)), - getOpacity: (d) => (isPointVisible(d) ? 1 : 0), - getPointerEvents: (d) => (isPointVisible(d) ? 'auto' : 'none'), + interactionRef.current.getCssColor( + interactionRef.current.resolveColor(d.hwKey as string), + ), + getOpacity: (d) => (interactionRef.current.isPointVisible(d) ? 1 : 0), + getPointerEvents: (d) => (interactionRef.current.isPointVisible(d) ? 'auto' : 'none'), hideLabels: hidePointLabels || showGradientLabels, getLabelText: (d) => (useAdvancedLabels ? getPointLabel(d) : String(d.tp)), foreground: 'var(--foreground)', @@ -1484,7 +1551,8 @@ const ScatterGraph = React.memo( 'hw-key': (d) => String(d.hwKey), precision: (d) => d.precision, }, - selectedPrecisions, + getShapeKey: (d) => + getShapeKeyForPrecision(d.precision, interactionRef.current.selectedPrecisions), }, keyFn: buildPointConfigId, }; @@ -1802,22 +1870,26 @@ const ScatterGraph = React.memo( xScale: ContinuousScale, yScale: ContinuousScale, ) => { + // Annotations / colors via the interaction ref: they change with the + // visible series, and the decoration effect re-runs this layer on + // toggles without recreating it. + const ir = interactionRef.current; renderKnownIssueAnnotations(ctx.layout.g, ctx.layout.defs, { chartId, width: ctx.width, height: ctx.height, xScale, yScale, - annotations: knownIssueAnnotations, + annotations: ir.knownIssueAnnotations, rightInset: measureLegendRightInset( chartId, ctx.layout.svg.node(), ctx.layout.margin.left, ctx.width, ), - background: getCssColor('--background'), - foreground: getCssColor('--foreground'), - mutedForeground: getCssColor('--muted-foreground'), + background: ir.getCssColor('--background'), + foreground: ir.getCssColor('--foreground'), + mutedForeground: ir.getCssColor('--muted-foreground'), onLinkClick: (a) => track('inference_known_issue_clicked', { hwKey: a.issue.hwKey, @@ -1839,8 +1911,12 @@ const ScatterGraph = React.memo( result.push(speedOverlayLayer); result.push(knownIssueLayer); return result; + // Interaction state (visibility, colors, precision shapes, known-issue + // annotations) is deliberately NOT a dependency: layer closures read it + // through interactionRef, and the decoration effect restyles the + // existing DOM when it changes. Only data/structure changes recreate + // the layers (and with them, the full chart render). }, [ - knownIssueAnnotations, rooflines, allPointLabelsByKey, showGradientLabels, @@ -1849,19 +1925,13 @@ const ScatterGraph = React.memo( showMinecraftOverlay, gradientColorByPoint, chartId, - effectiveActiveHwTypes, - selectedPrecisions, - getCssColor, - resolveColor, pointsData, - isPointVisible, hidePointLabels, useAdvancedLabels, buildPointConfigId, overlayData, processedOverlayData, overlayRooflines, - activeOverlayHwTypes, unofficialRunInfos, runIndexByUrl, hardwareConfig, @@ -1869,12 +1939,20 @@ const ScatterGraph = React.memo( yLabel, selectedYAxisMetric, chartDefinition, - chartDefinition.chartType, ]); + // Layers handle for the decoration effect — lets it re-run individual + // custom layer renders (rooflines/labels, known issues) without waiting + // for a full chart rebuild. + const layersRef = useRef(layers); + layersRef.current = layers; + // --- onRender: tracked rings, CSS transitions, log tick formatting, dblclick --- const onRender = useCallback( (ctx: RenderContext) => { + // Stash the render context for the decoration effect. + lastRenderCtxRef.current = ctx; + const ir = interactionRef.current; const { zoomGroup } = ctx.layout; // CSS transitions for smooth opacity animation on hw toggle @@ -1890,7 +1968,7 @@ const ScatterGraph = React.memo( .attr('class', 'tracked-ring') .attr('r', POINT_SIZE + 5) .attr('fill', 'none') - .attr('stroke', getCssColor(resolveColor(d.hwKey))) + .attr('stroke', ir.getCssColor(ir.resolveColor(d.hwKey))) .attr('stroke-width', 2) .attr('opacity', 0.7) .attr('pointer-events', 'none'); @@ -1908,6 +1986,7 @@ const ScatterGraph = React.memo( else addTrackedConfig(d, chartDefinition.chartType); // Update ring DOM immediately (onRender only runs inside the D3 effect) + const irNow = interactionRef.current; const group = d3.select(this); group .selectAll('.tracked-ring') @@ -1916,7 +1995,7 @@ const ScatterGraph = React.memo( .attr('class', 'tracked-ring') .attr('r', POINT_SIZE + 5) .attr('fill', 'none') - .attr('stroke', getCssColor(resolveColor(d.hwKey))) + .attr('stroke', irNow.getCssColor(irNow.resolveColor(d.hwKey))) .attr('stroke-width', 2) .attr('opacity', 0.7) .attr('pointer-events', 'none'); @@ -1956,6 +2035,72 @@ const ScatterGraph = React.memo( // --- Side effects --- + // Toggle decoration: restyle the existing DOM when visibility or colors + // change (legend hw toggles, precision toggles, optimal-only, high + // contrast, theme). This is the cheap "Effect 4" display-toggle path from + // docs/d3-charts.md — the full chart rebuild only runs when data or scale + // domains actually change. Runs after the D3 render effect in the same + // commit (child layout effects fire first), so on rebuild commits it's an + // idempotent re-apply. + useLayoutEffect(() => { + const svg = chartRef.current?.getSvgElement?.(); + const ctx = lastRenderCtxRef.current; + if (!svg || !ctx) return; + const ir = interactionRef.current; + const zoomGroup = d3.select(svg).select('.zoom-group'); + if (zoomGroup.empty()) return; + + // Position with the same scales the zoom handler would use: rescale the + // base scales by the current transform when the user is zoomed in. + const t = d3.zoomTransform(svg); + const zoomed = t.k !== 1 || t.x !== 0 || t.y !== 0; + const xScale = zoomed ? t.rescaleX(ctx.xScale as ContinuousScale) : ctx.xScale; + const yScale = zoomed ? t.rescaleY(ctx.yScale as ContinuousScale) : ctx.yScale; + const decorationCtx: RenderContext = { ...ctx, xScale, yScale }; + + // Dots: visibility, vendor recolor, precision shape, tracked-ring color. + // Hand-rolled rather than a full renderScatterPoints pass so we skip + // re-writing label text on every point (the expensive part of the join). + zoomGroup.selectAll('.dot-group').each(function (d) { + const sel = d3.select(this); + const visible = ir.isPointVisible(d); + sel.style('opacity', visible ? 1 : 0).style('pointer-events', visible ? 'auto' : 'none'); + const color = + (showGradientLabels && gradientColorByPoint.get(d)) || + ir.getCssColor(ir.resolveColor(d.hwKey as string)); + syncPointShape( + sel as unknown as d3.Selection, + getShapeKeyForPrecision(d.precision, ir.selectedPrecisions), + color, + ); + sel.select('.tracked-ring').attr('stroke', color); + }); + + // Rooflines, gradient defs, parallelism + line labels (incl. overlay run + // labels): re-run the layer render — stroke colors, visibility, and + // label placement all depend on the visible set. Known-issue + // annotations likewise follow the visible series. Layer renders are + // idempotent data joins; labels that newly enter here can sit above the + // dots until the next full rebuild, which is cosmetic only + // (pointer-events: none). + for (const key of ['rooflines', 'known-issues'] as const) { + const layer = layersRef.current.find((l) => l.key === key); + if (layer && layer.type === 'custom' && layer.render) { + layer.render(zoomGroup, decorationCtx); + } + } + }, [ + isPointVisible, + effectiveActiveHwTypes, + selectedPrecisions, + activeOverlayHwTypes, + getCssColor, + resolveColor, + knownIssueAnnotations, + showGradientLabels, + gradientColorByPoint, + ]); + // Dismiss tooltip on filter changes useEffect(() => { chartRef.current?.dismissTooltip(); @@ -2010,7 +2155,10 @@ const ScatterGraph = React.memo( ref={chartRef} chartId={chartId} - data={chartScaleData} + // Stable across toggles: the render effect keys on this for "data + // changed" rebuilds; scale domains come from x/yScaleConfig (computed + // from the visible points), and visibility is applied via opacity. + data={pointsData} margin={CHART_MARGIN} watermark={getChartWatermark(isUnofficialRun)} testId="scatter-graph" diff --git a/packages/app/src/hooks/useStableValue.test.ts b/packages/app/src/hooks/useStableValue.test.ts new file mode 100644 index 00000000..1c6de846 --- /dev/null +++ b/packages/app/src/hooks/useStableValue.test.ts @@ -0,0 +1,104 @@ +// @vitest-environment jsdom +import { act, createElement } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { describe, expect, it } from 'vitest'; + +import { useStableValue } from './useStableValue'; + +// Lightweight renderHook with re-renderable input — TLR isn't installed. +function renderHookWithInput( + hook: (props: P) => T, + initial: P, +): { result: { current: T }; rerender: (props: P) => void; unmount: () => void; root: Root } { + const result = { current: undefined as unknown as T }; + let props = initial; + function TestComponent() { + result.current = hook(props); + return null; + } + const container = document.createElement('div'); + document.body.append(container); + const root = createRoot(container); + const render = () => { + act(() => { + root.render(createElement(TestComponent)); + }); + }; + render(); + return { + result, + rerender: (next: P) => { + props = next; + render(); + }, + unmount: () => { + act(() => root.unmount()); + container.remove(); + }, + root, + }; +} + +interface Domainish { + type: string; + domain: [number, number]; +} + +const sameDomain = (a: Domainish, b: Domainish) => + a.type === b.type && a.domain[0] === b.domain[0] && a.domain[1] === b.domain[1]; + +describe('useStableValue', () => { + it('returns the initial value on first render', () => { + const value: Domainish = { type: 'linear', domain: [0, 105] }; + const { result, unmount } = renderHookWithInput( + (v: Domainish) => useStableValue(v, sameDomain), + value, + ); + expect(result.current).toBe(value); + unmount(); + }); + + it('preserves the previous reference when the new value is equal', () => { + const first: Domainish = { type: 'linear', domain: [0, 105] }; + const { result, rerender, unmount } = renderHookWithInput( + (v: Domainish) => useStableValue(v, sameDomain), + first, + ); + + // Recomputed object (e.g. scale config after a legend toggle) with the + // same value — downstream effects keyed on it must not re-fire. + rerender({ type: 'linear', domain: [0, 105] }); + expect(result.current).toBe(first); + unmount(); + }); + + it('adopts the new reference when the value changes', () => { + const first: Domainish = { type: 'linear', domain: [0, 105] }; + const { result, rerender, unmount } = renderHookWithInput( + (v: Domainish) => useStableValue(v, sameDomain), + first, + ); + + const changed: Domainish = { type: 'linear', domain: [0, 210] }; + rerender(changed); + expect(result.current).toBe(changed); + + // And keeps the adopted reference on subsequent equal values. + rerender({ type: 'linear', domain: [0, 210] }); + expect(result.current).toBe(changed); + unmount(); + }); + + it('treats a type change as a new value even with identical domains', () => { + const linear: Domainish = { type: 'linear', domain: [1, 100] }; + const { result, rerender, unmount } = renderHookWithInput( + (v: Domainish) => useStableValue(v, sameDomain), + linear, + ); + + const log: Domainish = { type: 'log', domain: [1, 100] }; + rerender(log); + expect(result.current).toBe(log); + unmount(); + }); +}); diff --git a/packages/app/src/hooks/useStableValue.ts b/packages/app/src/hooks/useStableValue.ts new file mode 100644 index 00000000..c31ef381 --- /dev/null +++ b/packages/app/src/hooks/useStableValue.ts @@ -0,0 +1,21 @@ +import { useRef } from 'react'; + +/** + * Returns the previous value as long as the new one is `isEqual` to it, + * preserving referential identity across re-renders. + * + * Use for derived config objects that are recomputed every render but rarely + * change by value (e.g. chart scale domains recomputed from visible points on + * every legend toggle). Downstream `useMemo`/`useEffect` hooks keyed on the + * object then only fire when the value actually changed. + * + * Same ref-stability technique as the sorted-hardware-config check in + * useChartData (see docs/pitfalls.md "Hardware Config Ref Stability"). + */ +export function useStableValue(value: T, isEqual: (prev: T, next: T) => boolean): T { + const ref = useRef(value); + if (ref.current !== value && !isEqual(ref.current, value)) { + ref.current = value; + } + return ref.current; +} diff --git a/packages/app/src/lib/d3-chart/layers/scatter-points.test.ts b/packages/app/src/lib/d3-chart/layers/scatter-points.test.ts new file mode 100644 index 00000000..debbb788 --- /dev/null +++ b/packages/app/src/lib/d3-chart/layers/scatter-points.test.ts @@ -0,0 +1,165 @@ +// @vitest-environment jsdom +import * as d3 from 'd3'; +import { describe, expect, it } from 'vitest'; + +import type { ShapeKey } from '@/lib/chart-rendering'; + +import { renderScatterPoints, syncPointShape } from './scatter-points'; + +interface TestPoint { + hwKey: string; + precision: string; + x: number; + y: number; + tp: number; +} + +const POINTS: TestPoint[] = [ + { hwKey: 'h100', precision: 'fp8', x: 10, y: 100, tp: 8 }, + { hwKey: 'h100', precision: 'fp4', x: 20, y: 200, tp: 8 }, + { hwKey: 'mi300x', precision: 'fp8', x: 30, y: 300, tp: 4 }, +]; + +function makeZoomGroup() { + const svg = d3.create('svg:svg'); + return svg.append('g') as d3.Selection; +} + +const xScale = d3.scaleLinear().domain([0, 100]).range([0, 800]); +const yScale = d3.scaleLinear().domain([0, 400]).range([600, 0]); + +const keyFn = (d: TestPoint) => `${d.hwKey}|${d.precision}`; + +describe('renderScatterPoints with getShapeKey', () => { + it('resolves shapes through the accessor', () => { + const group = makeZoomGroup(); + renderScatterPoints( + group, + POINTS, + xScale, + yScale, + { + getColor: () => '#123456', + getShapeKey: (d) => (d.precision === 'fp8' ? 'circle' : 'square'), + }, + keyFn, + ); + + const shapes = group.selectAll('.visible-shape').nodes(); + expect(shapes).toHaveLength(3); + expect(shapes.map((n) => n.tagName.toLowerCase()).toSorted()).toEqual([ + 'circle', + 'circle', + 'rect', + ]); + const fp4Shape = group + .selectAll('.dot-group') + .filter((d) => d.precision === 'fp4') + .select('.visible-shape'); + expect(fp4Shape.attr('data-shape-key')).toBe('square'); + }); + + it('swaps shape elements in place when the accessor result changes', () => { + const group = makeZoomGroup(); + // Single selected precision: everything is a circle. + const shapeState: { fp4: ShapeKey } = { fp4: 'circle' }; + const config = { + getColor: () => '#123456', + getShapeKey: (d: TestPoint) => (d.precision === 'fp4' ? shapeState.fp4 : 'circle'), + }; + + renderScatterPoints(group, POINTS, xScale, yScale, config, keyFn); + expect( + group + .selectAll('.visible-shape') + .nodes() + .every((n) => n.tagName.toLowerCase() === 'circle'), + ).toBe(true); + + // Second precision selected: fp4 points become squares. Same config + // object — the accessor reads current state, mirroring the ref-based + // accessors ScatterGraph passes so a precision toggle doesn't have to + // recreate the layer config. + shapeState.fp4 = 'square'; + renderScatterPoints(group, POINTS, xScale, yScale, config, keyFn); + + const dotGroups = group.selectAll('.dot-group'); + expect(dotGroups.size()).toBe(3); // reused, not recreated + const fp4Shape = dotGroups + .filter((d) => d.precision === 'fp4') + .select('.visible-shape'); + expect(fp4Shape.node()!.tagName.toLowerCase()).toBe('rect'); + expect(fp4Shape.attr('data-shape-key')).toBe('square'); + }); + + it('falls back to selectedPrecisions ordering when no accessor is given', () => { + const group = makeZoomGroup(); + renderScatterPoints( + group, + POINTS, + xScale, + yScale, + { + getColor: () => '#123456', + selectedPrecisions: ['fp8', 'fp4'], + }, + keyFn, + ); + + const byPrecision = (precision: string) => + group + .selectAll('.dot-group') + .filter((d) => d.precision === precision) + .select('.visible-shape') + .attr('data-shape-key'); + expect(byPrecision('fp8')).toBe('circle'); + expect(byPrecision('fp4')).toBe('square'); + }); +}); + +describe('syncPointShape', () => { + function makeDotGroup() { + const group = makeZoomGroup(); + return group.append('g').attr('class', 'dot-group') as d3.Selection< + SVGGElement, + unknown, + null, + undefined + >; + } + + it('creates the shape element when missing', () => { + const g = makeDotGroup(); + syncPointShape(g, 'circle', '#ff0000'); + const shape = g.select('.visible-shape'); + expect(shape.empty()).toBe(false); + expect(shape.node()!.tagName.toLowerCase()).toBe('circle'); + expect(shape.attr('fill')).toBe('#ff0000'); + expect(shape.attr('data-shape-key')).toBe('circle'); + }); + + it('updates fill in place when the shape type is unchanged', () => { + const g = makeDotGroup(); + syncPointShape(g, 'circle', '#ff0000'); + const node = g.select('.visible-shape').node(); + + syncPointShape(g, 'circle', '#00ff00'); + const shape = g.select('.visible-shape'); + expect(shape.node()).toBe(node); // same element, no swap + expect(shape.attr('fill')).toBe('#00ff00'); + }); + + it('swaps the element when the shape type changes', () => { + const g = makeDotGroup(); + syncPointShape(g, 'circle', '#ff0000'); + const before = g.select('.visible-shape').node(); + + syncPointShape(g, 'square', '#ff0000'); + const after = g.select('.visible-shape'); + expect(after.node()).not.toBe(before); + expect(after.node()!.tagName.toLowerCase()).toBe('rect'); + expect(after.attr('data-shape-key')).toBe('square'); + // Only one visible shape remains. + expect(g.selectAll('.visible-shape').size()).toBe(1); + }); +}); diff --git a/packages/app/src/lib/d3-chart/layers/scatter-points.ts b/packages/app/src/lib/d3-chart/layers/scatter-points.ts index 5a76bf5e..0c316366 100644 --- a/packages/app/src/lib/d3-chart/layers/scatter-points.ts +++ b/packages/app/src/lib/d3-chart/layers/scatter-points.ts @@ -25,6 +25,13 @@ export interface ScatterPointConfig { * Defaults to `[d.precision]` per-point (all points render as circles). */ selectedPrecisions?: readonly string[]; + /** + * Per-point shape resolver. Takes precedence over `selectedPrecisions`. + * Use when the shape mapping must stay current between layer-config + * recreations (e.g. read through a ref so a precision toggle doesn't have + * to rebuild the whole chart). + */ + getShapeKey?: (d: T) => ShapeKey; } const resolveShapeKey = (precision: string, selectedPrecisions?: readonly string[]): ShapeKey => @@ -32,6 +39,45 @@ const resolveShapeKey = (precision: string, selectedPrecisions?: readonly string ? getShapeKeyForPrecision(precision, selectedPrecisions) : 'circle'; +/** + * Ensure a dot-group's `.visible-shape` matches the target shape and fill. + * Swaps the SVG element (remove/append) when its tag needs to change, and + * stamps the shape key on the element so other code (e.g. useStickyTooltip's + * reset path) can restore normal-state attrs without knowing precisions. + * + * Shared by the full layer render and by ScatterGraph's lightweight toggle + * decoration pass, so both produce identical DOM. + */ +export function syncPointShape( + g: d3.Selection, + shapeKey: ShapeKey, + fill: string, +): void { + const targetType = getShapeConfig(shapeKey).type; + const existing = g.select('.visible-shape').node(); + const currentType = existing?.tagName.toLowerCase(); + if (!existing || currentType !== targetType) { + g.select('.visible-shape').remove(); + const shape = g + .append(targetType) + .attr('class', 'visible-shape') + .attr('data-shape-key', shapeKey) + .attr('fill', fill) + .attr('stroke', 'none') + .attr('cursor', 'pointer') as d3.Selection< + SVGCircleElement | SVGRectElement | SVGPathElement, + unknown, + null, + undefined + >; + applyNormalState(shape, shapeKey); + } else { + const shape = g.select('.visible-shape'); + shape.attr('fill', fill).attr('data-shape-key', shapeKey); + applyNormalState(shape as any, shapeKey); + } +} + /** * Render scatter points into a zoom group: group → hit area → shape → optional label. * Uses D3 enter/update/exit so existing DOM nodes are reused on data changes. @@ -95,37 +141,13 @@ export function renderScatterPoints('.visible-shape').node(); - const currentType = existing?.tagName.toLowerCase(); - if (!existing || currentType !== targetType) { - g.select('.visible-shape').remove(); - const shape = g - .append(targetType) - .attr('class', 'visible-shape') - .attr('data-shape-key', shapeKey) - .attr('fill', config.getColor(d)) - .attr('stroke', 'none') - .attr('cursor', 'pointer') as d3.Selection< - SVGCircleElement | SVGRectElement | SVGPathElement, - unknown, - null, - undefined - >; - applyNormalState(shape, shapeKey); - } else { - const shape = g.select('.visible-shape'); - shape.attr('fill', config.getColor(d)).attr('data-shape-key', shapeKey); - applyNormalState(shape as any, shapeKey); - } + const shapeKey = config.getShapeKey + ? config.getShapeKey(d) + : resolveShapeKey(d.precision, config.selectedPrecisions); + syncPointShape(g, shapeKey, config.getColor(d)); }); // Update labels: use data join so labels are created/removed properly on toggle From 1f4b86c59b6810c06665b7dd1787d7fef6b32c67 Mon Sep 17 00:00:00 2001 From: Oseltamivir <58582368+Oseltamivir@users.noreply.github.com> Date: Thu, 11 Jun 2026 22:27:48 -0700 Subject: [PATCH 2/2] fix(inference): keep roofline entrance animation on domain-changing toggles 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). --- .../ui/ScatterGraph.decoration.test.tsx | 37 ++++++ .../components/inference/ui/ScatterGraph.tsx | 109 ++++++++++++++---- 2 files changed, 123 insertions(+), 23 deletions(-) diff --git a/packages/app/src/components/inference/ui/ScatterGraph.decoration.test.tsx b/packages/app/src/components/inference/ui/ScatterGraph.decoration.test.tsx index 097355fb..a5c7ed47 100644 --- a/packages/app/src/components/inference/ui/ScatterGraph.decoration.test.tsx +++ b/packages/app/src/components/inference/ui/ScatterGraph.decoration.test.tsx @@ -11,6 +11,7 @@ import { act, createElement, useReducer } from 'react'; import { createRoot } from 'react-dom/client'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import * as d3 from 'd3'; import { setupChartStructure } from '@/lib/d3-chart/chart-setup'; import type { ChartDefinition, InferenceData } from '@/components/inference/types'; @@ -286,6 +287,42 @@ describe('ScatterGraph toggle decoration', () => { unmount(); }); + it('animates rooflines together with dots on a domain-changing toggle', () => { + // Real transition duration: the rebuild restores each surviving element to + // its old position/path and schedules a "data-update" transition that only + // starts on the next timer tick. Regression: the decoration effect used to + // re-apply final roofline `d` attrs in the same commit, so the curve + // teleported to its destination while the dots animated. + const { container, rerender, unmount } = mountChart({ transitionDuration: 750 }); + + const b200Roofline = () => + container.querySelector('.roofline-path[data-hw-key="b200"]')!; + const b200Dot = () => dotGroups(container, 'b200')[0]; + const dBefore = b200Roofline().getAttribute('d'); + const dotTransformBefore = b200Dot().getAttribute('transform'); + expect(dBefore).toBeTruthy(); + + // Hide the extreme-owning hw → domains shrink → full render + animation. + inferenceState.current = { + ...inferenceState.current, + activeHwTypes: new Set(['b200']), + }; + rerender(); + + // At commit end the transitions are scheduled but have not ticked: both + // the roofline path and the dots must still sit at their OLD coordinates, + // each with a pending transition toward the new ones. + expect(b200Roofline().getAttribute('d')).toBe(dBefore); + expect(b200Dot().getAttribute('transform')).toBe(dotTransformBefore); + expect((b200Roofline() as unknown as { __transition?: object }).__transition).toBeTruthy(); + expect((b200Dot() as unknown as { __transition?: object }).__transition).toBeTruthy(); + + // jsdom can't run the SVG transform interpolator (no transform.baseVal), + // so cancel the scheduled transitions before teardown. + d3.select(container).selectAll('.dot-group, .roofline-path').interrupt('data-update'); + unmount(); + }); + it('keeps unofficial-run overlay markers rendered through official toggles', () => { const overlayPoints = [point('h100', 'fp8', 30, 300, 2), point('h100', 'fp8', 35, 350, 4)].map( (p) => ({ ...p, run_url: 'https://github.com/o/r/actions/runs/123' }), diff --git a/packages/app/src/components/inference/ui/ScatterGraph.tsx b/packages/app/src/components/inference/ui/ScatterGraph.tsx index 266a2ca0..b78bb799 100644 --- a/packages/app/src/components/inference/ui/ScatterGraph.tsx +++ b/packages/app/src/components/inference/ui/ScatterGraph.tsx @@ -123,6 +123,18 @@ const isSameScaleConfig = (a: ScaleConfigValue, b: ScaleConfigValue): boolean => a.domain[0] === b.domain[0] && a.domain[1] === b.domain[1]; +// True when the node has a scheduled or running d3 transition with this name. +// Reads d3-transition's per-node schedule store (`__transition`) because +// d3.active() only reports transitions that already started, and the chart's +// entrance transitions are scheduled in the same commit but start on the next +// timer tick. +const hasNamedTransition = (node: Element, name: string): boolean => { + const schedules = (node as Element & { __transition?: Record }) + .__transition; + if (!schedules) return false; + return Object.values(schedules).some((schedule) => schedule?.name === name); +}; + // Derive a readable label from a hwKey using the HARDWARE_CONFIG source of truth const parseHwKeyToLabel = (hwKey: string): { name: string; label: string } => { const config = getHardwareConfig(hwKey); @@ -2039,9 +2051,14 @@ const ScatterGraph = React.memo( // change (legend hw toggles, precision toggles, optimal-only, high // contrast, theme). This is the cheap "Effect 4" display-toggle path from // docs/d3-charts.md — the full chart rebuild only runs when data or scale - // domains actually change. Runs after the D3 render effect in the same - // commit (child layout effects fire first), so on rebuild commits it's an - // idempotent re-apply. + // domains actually change. + // + // This effect can run in the same commit as (right after) the full render + // effect, while the renderer's old→new "data-update" entrance transitions + // are scheduled but not yet started. It therefore NEVER writes the + // attributes those transitions animate — dot-group `transform` and + // roofline `d` — or a freshly rebuilt roofline would start its transition + // already at the destination and teleport while the dots animate. useLayoutEffect(() => { const svg = chartRef.current?.getSvgElement?.(); const ctx = lastRenderCtxRef.current; @@ -2050,17 +2067,10 @@ const ScatterGraph = React.memo( const zoomGroup = d3.select(svg).select('.zoom-group'); if (zoomGroup.empty()) return; - // Position with the same scales the zoom handler would use: rescale the - // base scales by the current transform when the user is zoomed in. - const t = d3.zoomTransform(svg); - const zoomed = t.k !== 1 || t.x !== 0 || t.y !== 0; - const xScale = zoomed ? t.rescaleX(ctx.xScale as ContinuousScale) : ctx.xScale; - const yScale = zoomed ? t.rescaleY(ctx.yScale as ContinuousScale) : ctx.yScale; - const decorationCtx: RenderContext = { ...ctx, xScale, yScale }; - // Dots: visibility, vendor recolor, precision shape, tracked-ring color. // Hand-rolled rather than a full renderScatterPoints pass so we skip - // re-writing label text on every point (the expensive part of the join). + // re-writing label text on every point (the expensive part of the join) + // — and, critically, never touch the animated `transform`. zoomGroup.selectAll('.dot-group').each(function (d) { const sel = d3.select(this); const visible = ir.isPointVisible(d); @@ -2076,19 +2086,71 @@ const ScatterGraph = React.memo( sel.select('.tracked-ring').attr('stroke', color); }); - // Rooflines, gradient defs, parallelism + line labels (incl. overlay run - // labels): re-run the layer render — stroke colors, visibility, and - // label placement all depend on the visible set. Known-issue - // annotations likewise follow the visible series. Layer renders are - // idempotent data joins; labels that newly enter here can sit above the - // dots until the next full rebuild, which is cosmetic only - // (pointer-events: none). - for (const key of ['rooflines', 'known-issues'] as const) { - const layer = layersRef.current.find((l) => l.key === key); - if (layer && layer.type === 'custom' && layer.render) { - layer.render(zoomGroup, decorationCtx); + // Rooflines: visibility + solid-stroke recolor as direct writes (never + // `d`). Gradient strokes keep their url(#…) reference — gradient stop + // colors come from the fixed parallelism palette and don't change with + // the active set. + zoomGroup.selectAll('.roofline-path').each(function () { + const hw = this.dataset.hwKey; + const precision = this.dataset.precision; + if (!hw || !precision) return; + const el = d3.select(this); + const visible = + ir.effectiveActiveHwTypes.has(hw) && ir.selectedPrecisions.includes(precision); + el.style('opacity', visible ? 1 : 0); + const stroke = el.attr('stroke'); + if (stroke && !stroke.startsWith('url(')) { + el.attr('stroke', ir.getCssColor(ir.resolveColor(hw))); + } + }); + + // Parallelism / line labels: visibility via data attributes (mirrors + // handleLegendHoverEnd). Placement-level updates happen below. + zoomGroup + .selectAll('.parallelism-label, .line-label') + .style('opacity', function () { + const hw = (this as SVGGElement).dataset.hwKey; + const precision = (this as SVGGElement).dataset.precision; + if (!hw) return 0; + if (!precision) return ir.effectiveActiveHwTypes.has(hw) ? 1 : 0; + return ir.effectiveActiveHwTypes.has(hw) && ir.selectedPrecisions.includes(precision) + ? 1 + : 0; + }); + + // Label placement (greedy collision layout) depends on the visible set, + // so when labels are shown, re-run the rooflines layer render — UNLESS + // an entrance transition is still pending/running, because that render + // also rewrites roofline `d` and would defeat the animation. In that + // case the in-flight render was produced with current interaction state + // anyway; the direct writes above keep visibility correct. + const entranceInFlight = zoomGroup + .selectAll('.roofline-path') + .nodes() + .some((node) => hasNamedTransition(node, 'data-update')); + + // Current (possibly zoomed) scales for layer re-renders — same scales + // the zoom handler would use. + const t = d3.zoomTransform(svg); + const zoomed = t.k !== 1 || t.x !== 0 || t.y !== 0; + const xScale = zoomed ? t.rescaleX(ctx.xScale as ContinuousScale) : ctx.xScale; + const yScale = zoomed ? t.rescaleY(ctx.yScale as ContinuousScale) : ctx.yScale; + const decorationCtx: RenderContext = { ...ctx, xScale, yScale }; + + const layerByKey = (key: string) => layersRef.current.find((l) => l.key === key); + if ((showGradientLabels || showLineLabels) && !entranceInFlight) { + const rooflineLayer = layerByKey('rooflines'); + if (rooflineLayer?.type === 'custom' && rooflineLayer.render) { + rooflineLayer.render(zoomGroup, decorationCtx); } } + + // Known-issue annotations follow the visible series; their layer writes + // no animated attributes, so re-rendering is always safe. + const knownIssueLayer = layerByKey('known-issues'); + if (knownIssueLayer?.type === 'custom' && knownIssueLayer.render) { + knownIssueLayer.render(zoomGroup, decorationCtx); + } }, [ isPointVisible, effectiveActiveHwTypes, @@ -2098,6 +2160,7 @@ const ScatterGraph = React.memo( resolveColor, knownIssueAnnotations, showGradientLabels, + showLineLabels, gradientColorByPoint, ]);