Skip to content

Commit 4ecc47c

Browse files
perf(inference): decouple legend/precision toggles from full chart rebuild (#446)
* 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). * 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). --------- Co-authored-by: adibarra <93070681+adibarra@users.noreply.github.com>
1 parent e22c99c commit 4ecc47c

6 files changed

Lines changed: 961 additions & 76 deletions

File tree

Lines changed: 362 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,362 @@
1+
// @vitest-environment jsdom
2+
/**
3+
* Integration tests for the ScatterGraph toggle decoration path.
4+
*
5+
* Core behavior under test: legend hw toggles, precision toggles, and color
6+
* changes restyle the existing SVG (opacity / fill / shape) WITHOUT tearing
7+
* down and rebuilding the chart structure — the ~300ms main-thread long task
8+
* behind the failing field INP. A rebuild is detected via a spy on
9+
* setupChartStructure, which every full chart render must call.
10+
*/
11+
import { act, createElement, useReducer } from 'react';
12+
import { createRoot } from 'react-dom/client';
13+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
14+
import * as d3 from 'd3';
15+
16+
import { setupChartStructure } from '@/lib/d3-chart/chart-setup';
17+
import type { ChartDefinition, InferenceData } from '@/components/inference/types';
18+
19+
// ── Module mocks ───────────────────────────────────────────────────────────
20+
vi.mock('@/lib/d3-chart/chart-setup', { spy: true });
21+
vi.mock('@/lib/analytics', () => ({ track: vi.fn() }));
22+
vi.mock('next-themes', () => ({ useTheme: () => ({ resolvedTheme: 'dark' }) }));
23+
// The legend is React-rendered (covered elsewhere) — keep the tree light.
24+
vi.mock('@/components/ui/chart-legend', () => ({ default: () => null }));
25+
26+
const inferenceState = vi.hoisted(() => ({ current: {} as Record<string, unknown> }));
27+
vi.mock('@/components/inference/InferenceContext', () => ({
28+
useInference: () => inferenceState.current,
29+
}));
30+
31+
const overlayState = vi.hoisted(() => ({ current: {} as Record<string, unknown> }));
32+
vi.mock('@/components/unofficial-run-provider', () => ({
33+
useUnofficialRun: () => overlayState.current,
34+
}));
35+
36+
import ScatterGraph from './ScatterGraph';
37+
38+
// ── Environment stubs ────────────────────────────────────────────────────────
39+
class MockResizeObserver {
40+
observe() {}
41+
unobserve() {}
42+
disconnect() {}
43+
}
44+
45+
// ── Fixtures ─────────────────────────────────────────────────────────────────
46+
const point = (hwKey: string, precision: string, x: number, y: number, tp: number): InferenceData =>
47+
({ hwKey, precision, x, y, tp, conc: 16, framework: 'vllm' }) as unknown as InferenceData;
48+
49+
// h100 owns both axis extremes so hiding b200 / showing fp4 keeps the niced
50+
// domains identical — exactly the toggle case that must not rebuild.
51+
const POINTS: InferenceData[] = [
52+
point('h100', 'fp8', 1, 1, 1),
53+
point('h100', 'fp8', 100, 1000, 8),
54+
point('h100', 'fp4', 40, 400, 4),
55+
point('b200', 'fp8', 50, 500, 4),
56+
point('b200', 'fp8', 60, 600, 8),
57+
];
58+
59+
const HARDWARE_CONFIG = {
60+
h100: { name: 'H100', label: 'H100', gpu: 'H100' },
61+
b200: { name: 'B200', label: 'B200', gpu: 'B200' },
62+
};
63+
64+
const CHART_DEFINITION = { chartType: 'interactivity' } as unknown as ChartDefinition;
65+
66+
const noop = () => {};
67+
68+
function baseInferenceState() {
69+
return {
70+
activeHwTypes: new Set(['h100', 'b200']),
71+
hardwareConfig: HARDWARE_CONFIG,
72+
toggleHwType: noop,
73+
removeHwType: noop,
74+
hwTypesWithData: new Set(['h100', 'b200']),
75+
selectedPrecisions: ['fp8'],
76+
selectedYAxisMetric: 'y',
77+
availableRuns: null,
78+
selectedRunId: '',
79+
hideNonOptimal: false,
80+
setHideNonOptimal: noop,
81+
hidePointLabels: false,
82+
setHidePointLabels: noop,
83+
selectAllHwTypes: noop,
84+
highContrast: false,
85+
setHighContrast: noop,
86+
logScale: false,
87+
setLogScale: noop,
88+
scaleType: 'auto',
89+
isLegendExpanded: false,
90+
setIsLegendExpanded: noop,
91+
useAdvancedLabels: false,
92+
setUseAdvancedLabels: noop,
93+
showGradientLabels: false,
94+
setShowGradientLabels: noop,
95+
showLineLabels: false,
96+
setShowLineLabels: noop,
97+
showSpeedOverlay: false,
98+
setShowSpeedOverlay: noop,
99+
showMinecraftOverlay: false,
100+
setShowMinecraftOverlay: noop,
101+
trackedConfigs: [],
102+
addTrackedConfig: noop,
103+
removeTrackedConfig: noop,
104+
};
105+
}
106+
107+
function baseOverlayState() {
108+
return {
109+
isUnofficialRun: false,
110+
activeOverlayHwTypes: new Set<string>(),
111+
setActiveOverlayHwTypes: noop,
112+
allOverlayHwTypes: new Set<string>(),
113+
toggleOverlayHwType: noop,
114+
resetOverlayHwTypes: noop,
115+
localOfficialOverride: null,
116+
setLocalOfficialOverride: noop,
117+
runIndexByUrl: {},
118+
unofficialRunInfos: [],
119+
};
120+
}
121+
122+
// ── Harness ──────────────────────────────────────────────────────────────────
123+
function mountChart(props?: Partial<Parameters<typeof ScatterGraph>[0]>) {
124+
let forceUpdate: () => void = noop;
125+
function Harness() {
126+
// ScatterGraph and D3Chart are React.memo'd; mocked context hooks bypass
127+
// React's context subscription, so re-renders are driven through a
128+
// version-bumped caption prop.
129+
const [version, bump] = useReducer((v: number) => v + 1, 0);
130+
forceUpdate = bump;
131+
return createElement(ScatterGraph, {
132+
chartId: 'chart-test',
133+
modelLabel: 'DeepSeek-R1-0528',
134+
data: POINTS,
135+
xLabel: 'Interactivity (tok/s/user)',
136+
yLabel: 'Output Throughput per GPU',
137+
chartDefinition: CHART_DEFINITION,
138+
transitionDuration: 0,
139+
caption: `v${version}`,
140+
...props,
141+
});
142+
}
143+
144+
const container = document.createElement('div');
145+
document.body.append(container);
146+
const root = createRoot(container);
147+
act(() => {
148+
root.render(createElement(Harness));
149+
});
150+
return {
151+
container,
152+
rerender: () => act(() => forceUpdate()),
153+
unmount: () => {
154+
act(() => root.unmount());
155+
container.remove();
156+
},
157+
};
158+
}
159+
160+
const dotGroups = (container: HTMLElement, hwKey?: string) =>
161+
[...container.querySelectorAll<SVGGElement>('.dot-group')].filter(
162+
(n) => !hwKey || n.dataset.hwKey === hwKey,
163+
);
164+
165+
const rebuildCount = () => vi.mocked(setupChartStructure).mock.calls.length;
166+
167+
beforeEach(() => {
168+
vi.stubGlobal('ResizeObserver', MockResizeObserver);
169+
// Charts measure their container; jsdom reports 0 — give them real space.
170+
vi.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockReturnValue({
171+
width: 800,
172+
height: 600,
173+
top: 0,
174+
left: 0,
175+
right: 800,
176+
bottom: 600,
177+
x: 0,
178+
y: 0,
179+
toJSON: () => ({}),
180+
} as DOMRect);
181+
inferenceState.current = baseInferenceState();
182+
overlayState.current = baseOverlayState();
183+
vi.mocked(setupChartStructure).mockClear();
184+
});
185+
186+
afterEach(() => {
187+
vi.unstubAllGlobals();
188+
vi.restoreAllMocks();
189+
});
190+
191+
describe('ScatterGraph toggle decoration', () => {
192+
it('renders all points and rooflines after mount', () => {
193+
const { container, unmount } = mountChart();
194+
195+
expect(dotGroups(container)).toHaveLength(POINTS.length);
196+
expect(container.querySelectorAll('.roofline-path').length).toBeGreaterThan(0);
197+
expect(rebuildCount()).toBeGreaterThan(0);
198+
unmount();
199+
});
200+
201+
it('hides a toggled-off hw via opacity without rebuilding the chart', () => {
202+
const { container, rerender, unmount } = mountChart();
203+
const buildsAfterMount = rebuildCount();
204+
205+
inferenceState.current = {
206+
...inferenceState.current,
207+
activeHwTypes: new Set(['h100']),
208+
};
209+
rerender();
210+
211+
for (const dot of dotGroups(container, 'b200')) {
212+
expect(dot.style.opacity).toBe('0');
213+
expect(dot.style.pointerEvents).toBe('none');
214+
}
215+
for (const dot of dotGroups(container, 'h100').filter((d) => d.dataset.precision === 'fp8')) {
216+
expect(dot.style.opacity).toBe('1');
217+
expect(dot.style.pointerEvents).toBe('auto');
218+
}
219+
const b200Roofline = container.querySelector<SVGPathElement>(
220+
'.roofline-path[data-hw-key="b200"]',
221+
);
222+
expect(b200Roofline).not.toBeNull();
223+
expect(b200Roofline!.style.opacity).toBe('0');
224+
225+
// The whole point: a legend toggle is a restyle, not a teardown/rebuild.
226+
expect(rebuildCount()).toBe(buildsAfterMount);
227+
unmount();
228+
});
229+
230+
it('recolors remaining series when the active set changes, without rebuilding', () => {
231+
const { container, rerender, unmount } = mountChart();
232+
const buildsAfterMount = rebuildCount();
233+
const h100Fill = () =>
234+
dotGroups(container, 'h100')[0].querySelector('.visible-shape')!.getAttribute('fill');
235+
const before = h100Fill();
236+
237+
// h100 and b200 share the NVIDIA hue zone: dropping one redistributes
238+
// the remaining hues (dynamic-colors), so the dots must actually recolor.
239+
inferenceState.current = {
240+
...inferenceState.current,
241+
activeHwTypes: new Set(['h100']),
242+
};
243+
rerender();
244+
245+
expect(h100Fill()).not.toBe(before);
246+
expect(rebuildCount()).toBe(buildsAfterMount);
247+
unmount();
248+
});
249+
250+
it('swaps point shapes when a second precision is selected, without rebuilding', () => {
251+
const { container, rerender, unmount } = mountChart();
252+
const buildsAfterMount = rebuildCount();
253+
const fp4Dot = () => dotGroups(container, 'h100').find((d) => d.dataset.precision === 'fp4')!;
254+
255+
// Single precision: fp4 points are hidden circles.
256+
expect(fp4Dot().style.opacity).toBe('0');
257+
expect(fp4Dot().querySelector('.visible-shape')!.tagName.toLowerCase()).toBe('circle');
258+
259+
inferenceState.current = {
260+
...inferenceState.current,
261+
selectedPrecisions: ['fp8', 'fp4'],
262+
};
263+
rerender();
264+
265+
// Second precision becomes visible as the square shape (slot 2).
266+
expect(fp4Dot().style.opacity).toBe('1');
267+
const shape = fp4Dot().querySelector<SVGElement>('.visible-shape')!;
268+
expect(shape.tagName.toLowerCase()).toBe('rect');
269+
expect(shape.dataset.shapeKey).toBe('square');
270+
expect(rebuildCount()).toBe(buildsAfterMount);
271+
unmount();
272+
});
273+
274+
it('rebuilds when the scale domain actually changes (data refresh path intact)', () => {
275+
const { rerender, unmount } = mountChart();
276+
const buildsAfterMount = rebuildCount();
277+
278+
// Hiding the hw that owns the axis extremes changes the visible domain —
279+
// axes must rescale, which is a legitimate full render.
280+
inferenceState.current = {
281+
...inferenceState.current,
282+
activeHwTypes: new Set(['b200']),
283+
};
284+
rerender();
285+
286+
expect(rebuildCount()).toBeGreaterThan(buildsAfterMount);
287+
unmount();
288+
});
289+
290+
it('animates rooflines together with dots on a domain-changing toggle', () => {
291+
// Real transition duration: the rebuild restores each surviving element to
292+
// its old position/path and schedules a "data-update" transition that only
293+
// starts on the next timer tick. Regression: the decoration effect used to
294+
// re-apply final roofline `d` attrs in the same commit, so the curve
295+
// teleported to its destination while the dots animated.
296+
const { container, rerender, unmount } = mountChart({ transitionDuration: 750 });
297+
298+
const b200Roofline = () =>
299+
container.querySelector<SVGPathElement>('.roofline-path[data-hw-key="b200"]')!;
300+
const b200Dot = () => dotGroups(container, 'b200')[0];
301+
const dBefore = b200Roofline().getAttribute('d');
302+
const dotTransformBefore = b200Dot().getAttribute('transform');
303+
expect(dBefore).toBeTruthy();
304+
305+
// Hide the extreme-owning hw → domains shrink → full render + animation.
306+
inferenceState.current = {
307+
...inferenceState.current,
308+
activeHwTypes: new Set(['b200']),
309+
};
310+
rerender();
311+
312+
// At commit end the transitions are scheduled but have not ticked: both
313+
// the roofline path and the dots must still sit at their OLD coordinates,
314+
// each with a pending transition toward the new ones.
315+
expect(b200Roofline().getAttribute('d')).toBe(dBefore);
316+
expect(b200Dot().getAttribute('transform')).toBe(dotTransformBefore);
317+
expect((b200Roofline() as unknown as { __transition?: object }).__transition).toBeTruthy();
318+
expect((b200Dot() as unknown as { __transition?: object }).__transition).toBeTruthy();
319+
320+
// jsdom can't run the SVG transform interpolator (no transform.baseVal),
321+
// so cancel the scheduled transitions before teardown.
322+
d3.select(container).selectAll('.dot-group, .roofline-path').interrupt('data-update');
323+
unmount();
324+
});
325+
326+
it('keeps unofficial-run overlay markers rendered through official toggles', () => {
327+
const overlayPoints = [point('h100', 'fp8', 30, 300, 2), point('h100', 'fp8', 35, 350, 4)].map(
328+
(p) => ({ ...p, run_url: 'https://github.com/o/r/actions/runs/123' }),
329+
);
330+
overlayState.current = {
331+
...baseOverlayState(),
332+
isUnofficialRun: true,
333+
activeOverlayHwTypes: new Set(['h100']),
334+
allOverlayHwTypes: new Set(['h100']),
335+
runIndexByUrl: { 'https://github.com/o/r/actions/runs/123': 0 },
336+
unofficialRunInfos: [
337+
{ id: '123', branch: 'test-branch', url: 'https://github.com/o/r/actions/runs/123' },
338+
],
339+
};
340+
const { container, rerender, unmount } = mountChart({
341+
overlayData: {
342+
data: overlayPoints,
343+
hardwareConfig: HARDWARE_CONFIG,
344+
} as unknown as Parameters<typeof ScatterGraph>[0]['overlayData'],
345+
});
346+
const buildsAfterMount = rebuildCount();
347+
348+
expect(container.querySelectorAll('.unofficial-overlay-pt')).toHaveLength(2);
349+
expect(container.querySelectorAll('.overlay-roofline-path').length).toBeGreaterThan(0);
350+
351+
// Toggling an official hw must not rebuild or disturb overlay markers.
352+
inferenceState.current = {
353+
...inferenceState.current,
354+
activeHwTypes: new Set(['h100']),
355+
};
356+
rerender();
357+
358+
expect(container.querySelectorAll('.unofficial-overlay-pt')).toHaveLength(2);
359+
expect(rebuildCount()).toBe(buildsAfterMount);
360+
unmount();
361+
});
362+
});

0 commit comments

Comments
 (0)