diff --git a/.claude/skills/perf-fix-with-proof/SKILL.md b/.claude/skills/perf-fix-with-proof/SKILL.md new file mode 100644 index 0000000000..56ba55d2e5 --- /dev/null +++ b/.claude/skills/perf-fix-with-proof/SKILL.md @@ -0,0 +1,179 @@ +--- +name: perf-fix-with-proof +description: 'Ships performance fixes with CI-proven improvement using stacked PRs. PR1 adds a @perf test (establishes baseline on main), PR2 adds the fix (CI shows delta). Use when implementing a perf optimization and wanting to prove it in CI.' +--- + +# Performance Fix with Proof + +Ships perf fixes as two stacked PRs so CI automatically proves the improvement. + +## Why Two PRs + +The `ci-perf-report.yaml` workflow compares PR metrics against the **base branch baseline**. If you add a new `@perf` test in the same PR as the fix, that test doesn't exist on main yet — no baseline, no delta, no proof. Stacking solves this: + +1. **PR1 (test-only)** — adds the `@perf` test that exercises the bottleneck. Merges to main. CI runs it on main → baseline established. +2. **PR2 (fix)** — adds the optimization. CI runs the same test → compares against PR1's baseline → delta shows improvement. + +## Workflow + +### Step 1: Create the test branch + +```bash +git worktree add -b perf/test- origin/main +``` + +### Step 2: Write the `@perf` test + +Add a test to `browser_tests/tests/performance.spec.ts` (or a new file with `@perf` tag). The test should stress the specific bottleneck. + +**Test structure:** + +```typescript +test('', async ({ comfyPage }) => { + // 1. Load a workflow that exercises the bottleneck + await comfyPage.workflow.loadWorkflow('') + + // 2. Start measuring + await comfyPage.perf.startMeasuring() + + // 3. Perform the action that triggers the bottleneck (at scale) + for (let i = 0; i < N; i++) { + // ... stress the hot path ... + await comfyPage.nextFrame() + } + + // 4. Stop measuring and record + const m = await comfyPage.perf.stopMeasuring('') + recordMeasurement(m) + console.log(`: ${m.styleRecalcs} recalcs, ${m.layouts} layouts`) +}) +``` + +**Available metrics** (from `PerformanceHelper`): + +- `m.styleRecalcs` / `m.styleRecalcDurationMs` — style recalculation count and time +- `m.layouts` / `m.layoutDurationMs` — forced layout count and time +- `m.taskDurationMs` — total main-thread JS execution time +- `m.heapDeltaBytes` — memory pressure delta + +**Key helpers** (from `ComfyPage`): + +- `comfyPage.perf.startMeasuring()` / `.stopMeasuring(name)` — CDP metrics capture +- `comfyPage.nextFrame()` — wait one animation frame +- `comfyPage.workflow.loadWorkflow(name)` — load a test workflow from `browser_tests/assets/` +- `comfyPage.canvas` — the canvas locator +- `comfyPage.page.mouse.move(x, y)` — mouse interaction + +### Step 3: Add test workflow asset (if needed) + +If the bottleneck needs a specific workflow (e.g., 50+ nodes, many DOM widgets), add it to `browser_tests/assets/`. Keep it minimal — only the structure needed to trigger the bottleneck. + +### Step 4: Verify locally + +```bash +pnpm exec playwright test --project=performance --grep "" +``` + +Confirm the test runs and produces reasonable metric values. + +### Step 5: Create PR1 (test-only) + +```bash +pnpm typecheck:browser +pnpm lint +git add browser_tests/ +git commit -m "test: add perf test for " +git push -u origin perf/test- +gh pr create --title "test: add perf test for " \ + --body "Adds a @perf test to establish a baseline for . + +This is PR 1 of 2. The fix will follow in a separate PR once this baseline is established on main. + +## What +Adds \`\` to the performance test suite measuring during . + +## Why +Needed to prove the improvement from the upcoming fix for backlog item #." \ + --base main +``` + +### Step 6: Get PR1 merged + +Once PR1 merges, CI runs the test on main → baseline artifact saved. + +### Step 7: Create PR2 (fix) on top of main + +```bash +git worktree add -b perf/fix- origin/main +``` + +Implement the fix. The `@perf` test from PR1 is now on main and will run automatically. CI will: + +1. Run the test on the PR branch +2. Download the baseline from main (which includes PR1's test results) +3. Post a PR comment showing the delta + +### Step 8: Verify the improvement shows in CI + +The `ci-perf-report.yaml` posts a comment like: + +```markdown +## ⚡ Performance Report + +| Metric | Baseline | PR (n=3) | Δ | Sig | +| --------------------- | -------- | -------- | ---- | --- | +| : style recalcs | 450 | 12 | -97% | 🟢 | +``` + +If Δ is negative for the target metric, the fix is proven. + +## Test Design Guidelines + +1. **Stress the specific bottleneck** — don't measure everything, isolate the hot path +2. **Use enough iterations** — the test should run long enough that the metric difference is clear (100+ frames for idle tests, 50+ interactions for event tests) +3. **Keep it deterministic** — avoid timing-dependent assertions; measure counts not durations when possible +4. **Match the backlog entry** — reference the backlog item number in the test name or PR description + +## Examples + +**Testing DOM widget reactive mutations (backlog #8):** + +```typescript +test('DOM widget positioning recalculations', async ({ comfyPage }) => { + await comfyPage.workflow.loadWorkflow('default') + await comfyPage.perf.startMeasuring() + // Idle for 120 frames — DOM widgets update position every frame + for (let i = 0; i < 120; i++) { + await comfyPage.nextFrame() + } + const m = await comfyPage.perf.stopMeasuring('dom-widget-idle') + recordMeasurement(m) +}) +``` + +**Testing measureText caching (backlog #4):** + +```typescript +test('canvas text rendering with many nodes', async ({ comfyPage }) => { + await comfyPage.workflow.loadWorkflow('large-workflow-50-nodes') + await comfyPage.perf.startMeasuring() + for (let i = 0; i < 60; i++) { + await comfyPage.nextFrame() + } + const m = await comfyPage.perf.stopMeasuring('text-rendering-50-nodes') + recordMeasurement(m) +}) +``` + +## Reference + +| Resource | Path | +| ----------------- | ----------------------------------------------------- | +| Perf test file | `browser_tests/tests/performance.spec.ts` | +| PerformanceHelper | `browser_tests/fixtures/helpers/PerformanceHelper.ts` | +| Perf reporter | `browser_tests/helpers/perfReporter.ts` | +| CI workflow | `.github/workflows/ci-perf-report.yaml` | +| Report generator | `scripts/perf-report.ts` | +| Stats utilities | `scripts/perf-stats.ts` | +| Backlog | `docs/perf/BACKLOG.md` (local only, not committed) | +| Playbook | `docs/perf/PLAYBOOK.md` (local only, not committed) | diff --git a/src/lib/litegraph/src/LGraphBadge.ts b/src/lib/litegraph/src/LGraphBadge.ts index 6fad0a09c0..25005ce94d 100644 --- a/src/lib/litegraph/src/LGraphBadge.ts +++ b/src/lib/litegraph/src/LGraphBadge.ts @@ -1,6 +1,7 @@ import type { ReadOnlyRect } from '@/lib/litegraph/src/interfaces' import { LGraphIcon } from './LGraphIcon' import type { LGraphIconOptions } from './LGraphIcon' +import { cachedMeasureText } from './utils/textMeasureCache' export enum BadgePosition { TopLeft = 'top-left', @@ -80,11 +81,11 @@ export class LGraphBadge { iconWidth = this.icon.size + this.padding } else if (this.icon.unicode) { ctx.font = `${this.icon.fontSize}px '${this.icon.fontFamily}'` - iconWidth = ctx.measureText(this.icon.unicode).width + this.padding + iconWidth = cachedMeasureText(ctx, this.icon.unicode) + this.padding } } ctx.font = `${this.fontSize}px sans-serif` - const textWidth = this.text ? ctx.measureText(this.text).width : 0 + const textWidth = this.text ? cachedMeasureText(ctx, this.text) : 0 ctx.font = font return iconWidth + textWidth + this.padding * 2 } diff --git a/src/lib/litegraph/src/LGraphButton.ts b/src/lib/litegraph/src/LGraphButton.ts index b790368b94..99b0ca4832 100644 --- a/src/lib/litegraph/src/LGraphButton.ts +++ b/src/lib/litegraph/src/LGraphButton.ts @@ -1,6 +1,7 @@ import { LGraphBadge } from './LGraphBadge' import type { LGraphBadgeOptions } from './LGraphBadge' import { Rectangle } from './infrastructure/Rectangle' +import { cachedMeasureText } from './utils/textMeasureCache' export interface LGraphButtonOptions extends LGraphBadgeOptions { name?: string // To identify the button @@ -22,7 +23,7 @@ export class LGraphButton extends LGraphBadge { ctx.font = `${this.fontSize}px 'PrimeIcons'` // For icon buttons, just measure the text width without padding - const textWidth = this.text ? ctx.measureText(this.text).width : 0 + const textWidth = this.text ? cachedMeasureText(ctx, this.text) : 0 ctx.font = font return textWidth diff --git a/src/lib/litegraph/src/LGraphCanvas.ts b/src/lib/litegraph/src/LGraphCanvas.ts index 5ecd5bfebc..f6f2f4355e 100644 --- a/src/lib/litegraph/src/LGraphCanvas.ts +++ b/src/lib/litegraph/src/LGraphCanvas.ts @@ -26,6 +26,10 @@ import type { RerouteId } from './Reroute' import { LinkConnector } from './canvas/LinkConnector' import { isOverNodeInput, isOverNodeOutput } from './canvas/measureSlots' import { strokeShape } from './draw' +import { + cachedMeasureText, + clearTextMeasureCache +} from './utils/textMeasureCache' import type { CustomEventDispatcher, ICustomEventTarget @@ -4893,6 +4897,7 @@ export class LGraphCanvas implements CustomEventDispatcher * draws the front canvas (the one containing all the nodes) */ drawFrontCanvas(): void { + clearTextMeasureCache() this.dirty_canvas = false const { ctx, canvas, graph } = this @@ -5622,8 +5627,7 @@ export class LGraphCanvas implements CustomEventDispatcher text = text.substring(0, 30) ctx.font = '14px Courier New' - const info = ctx.measureText(text) - const w = info.width + 20 + const w = cachedMeasureText(ctx, text) + 20 const h = 24 ctx.shadowColor = 'black' ctx.shadowOffsetX = 2 diff --git a/src/lib/litegraph/src/LGraphNode.ts b/src/lib/litegraph/src/LGraphNode.ts index 99eb1c14d5..82e8f3eca8 100644 --- a/src/lib/litegraph/src/LGraphNode.ts +++ b/src/lib/litegraph/src/LGraphNode.ts @@ -17,6 +17,7 @@ import { } from '@/lib/litegraph/src/utils/type' import { SUBGRAPH_OUTPUT_ID } from '@/lib/litegraph/src/constants' +import { cachedMeasureText } from '@/lib/litegraph/src/utils/textMeasureCache' import type { DragAndScale } from './DragAndScale' import type { LGraph } from './LGraph' import { BadgePosition, LGraphBadge } from './LGraphBadge' @@ -2083,7 +2084,7 @@ export class LGraphNode this._collapsed_width = Math.min( this.size[0], ctx - ? ctx.measureText(this.getTitle() ?? '').width + + ? cachedMeasureText(ctx, this.getTitle() ?? '') + LiteGraph.NODE_TITLE_HEIGHT * 2 : 0 ) diff --git a/src/lib/litegraph/src/draw.ts b/src/lib/litegraph/src/draw.ts index 70c2e1d715..a1f402277f 100644 --- a/src/lib/litegraph/src/draw.ts +++ b/src/lib/litegraph/src/draw.ts @@ -2,6 +2,7 @@ import type { Rectangle } from './infrastructure/Rectangle' import type { CanvasColour } from './interfaces' import { LiteGraph } from './litegraph' import { RenderShape, TitleMode } from './types/globalEnums' +import { cachedMeasureText } from './utils/textMeasureCache' const ELLIPSIS = '\u2026' const TWO_DOT_LEADER = '\u2025' @@ -161,17 +162,17 @@ function truncateTextToWidth( if (!(maxWidth > 0)) return '' // Text fits - const fullWidth = ctx.measureText(text).width + const fullWidth = cachedMeasureText(ctx, text) if (fullWidth <= maxWidth) return text - const ellipsisWidth = ctx.measureText(ELLIPSIS).width * 0.75 + const ellipsisWidth = cachedMeasureText(ctx, ELLIPSIS) * 0.75 // Can't even fit ellipsis if (ellipsisWidth > maxWidth) { - const twoDotsWidth = ctx.measureText(TWO_DOT_LEADER).width * 0.75 + const twoDotsWidth = cachedMeasureText(ctx, TWO_DOT_LEADER) * 0.75 if (twoDotsWidth < maxWidth) return TWO_DOT_LEADER - const oneDotWidth = ctx.measureText(ONE_DOT_LEADER).width * 0.75 + const oneDotWidth = cachedMeasureText(ctx, ONE_DOT_LEADER) * 0.75 return oneDotWidth < maxWidth ? ONE_DOT_LEADER : '' } @@ -190,7 +191,7 @@ function truncateTextToWidth( } const sub = text.substring(0, mid) - const currentWidth = ctx.measureText(sub).width + ellipsisWidth + const currentWidth = cachedMeasureText(ctx, sub) + ellipsisWidth if (currentWidth <= maxWidth) { // This length fits, try potentially longer @@ -217,7 +218,7 @@ export function drawTextInArea({ const { left, right, bottom, width, centreX } = area // Text already fits - const fullWidth = ctx.measureText(text).width + const fullWidth = cachedMeasureText(ctx, text) if (fullWidth <= width) { ctx.textAlign = align const x = align === 'left' ? left : align === 'right' ? right : centreX @@ -237,5 +238,5 @@ export function drawTextInArea({ // Draw the ellipsis, right-aligned to the button ctx.textAlign = 'right' const ellipsis = truncated.at(-1)! - ctx.fillText(ellipsis, right, bottom, ctx.measureText(ellipsis).width * 0.75) + ctx.fillText(ellipsis, right, bottom, cachedMeasureText(ctx, ellipsis) * 0.75) } diff --git a/src/lib/litegraph/src/utils/textMeasureCache.test.ts b/src/lib/litegraph/src/utils/textMeasureCache.test.ts new file mode 100644 index 0000000000..fdb5782dfd --- /dev/null +++ b/src/lib/litegraph/src/utils/textMeasureCache.test.ts @@ -0,0 +1,63 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import { cachedMeasureText, clearTextMeasureCache } from './textMeasureCache' + +function createMockCtx(font = '12px sans-serif'): CanvasRenderingContext2D { + return { + font, + measureText: vi.fn((text: string) => ({ width: text.length * 7 })) + } as unknown as CanvasRenderingContext2D +} + +describe('textMeasureCache', () => { + beforeEach(() => { + clearTextMeasureCache() + }) + + it('returns the measured width', () => { + const ctx = createMockCtx() + const width = cachedMeasureText(ctx, 'hello') + expect(width).toBe(35) + expect(ctx.measureText).toHaveBeenCalledWith('hello') + }) + + it('returns cached result on second call without re-measuring', () => { + const ctx = createMockCtx() + const first = cachedMeasureText(ctx, 'hello') + const second = cachedMeasureText(ctx, 'hello') + + expect(first).toBe(second) + expect(ctx.measureText).toHaveBeenCalledTimes(1) + }) + + it('uses font as part of the cache key', () => { + const ctx1 = createMockCtx('12px sans-serif') + const ctx2 = createMockCtx('24px monospace') + + cachedMeasureText(ctx1, 'hello') + cachedMeasureText(ctx2, 'hello') + + expect(ctx1.measureText).toHaveBeenCalledTimes(1) + expect(ctx2.measureText).toHaveBeenCalledTimes(1) + }) + + it('clearTextMeasureCache resets the cache', () => { + const ctx = createMockCtx() + cachedMeasureText(ctx, 'hello') + expect(ctx.measureText).toHaveBeenCalledTimes(1) + + clearTextMeasureCache() + + cachedMeasureText(ctx, 'hello') + expect(ctx.measureText).toHaveBeenCalledTimes(2) + }) + + it('caches different text strings separately', () => { + const ctx = createMockCtx() + const w1 = cachedMeasureText(ctx, 'abc') + const w2 = cachedMeasureText(ctx, 'abcd') + + expect(w1).not.toBe(w2) + expect(ctx.measureText).toHaveBeenCalledTimes(2) + }) +}) diff --git a/src/lib/litegraph/src/utils/textMeasureCache.ts b/src/lib/litegraph/src/utils/textMeasureCache.ts new file mode 100644 index 0000000000..709ece0691 --- /dev/null +++ b/src/lib/litegraph/src/utils/textMeasureCache.ts @@ -0,0 +1,18 @@ +const cache = new Map() + +export function cachedMeasureText( + ctx: CanvasRenderingContext2D, + text: string +): number { + const key = `${ctx.font}\0${text}` + const cached = cache.get(key) + if (cached !== undefined) return cached + + const width = ctx.measureText(text).width + cache.set(key, width) + return width +} + +export function clearTextMeasureCache(): void { + cache.clear() +} diff --git a/src/lib/litegraph/src/utils/textUtils.ts b/src/lib/litegraph/src/utils/textUtils.ts index d3bc0c49b8..11db10a8a4 100644 --- a/src/lib/litegraph/src/utils/textUtils.ts +++ b/src/lib/litegraph/src/utils/textUtils.ts @@ -1,3 +1,5 @@ +import { cachedMeasureText } from './textMeasureCache' + /** * Truncates text to fit within a given width using binary search for optimal performance. * @param ctx The canvas rendering context used for text measurement @@ -12,13 +14,13 @@ export function truncateText( maxWidth: number, ellipsis: string = '...' ): string { - const textWidth = ctx.measureText(text).width + const textWidth = cachedMeasureText(ctx, text) if (textWidth <= maxWidth || maxWidth <= 0) { return text } - const ellipsisWidth = ctx.measureText(ellipsis).width + const ellipsisWidth = cachedMeasureText(ctx, ellipsis) const availableWidth = maxWidth - ellipsisWidth if (availableWidth <= 0) { @@ -33,7 +35,7 @@ export function truncateText( while (low <= high) { const mid = Math.floor((low + high) / 2) const testText = text.substring(0, mid) - const testWidth = ctx.measureText(testText).width + const testWidth = cachedMeasureText(ctx, testText) if (testWidth <= availableWidth) { bestFit = mid diff --git a/src/lib/litegraph/src/widgets/BaseWidget.ts b/src/lib/litegraph/src/widgets/BaseWidget.ts index b8138af707..2b2daa0b9e 100644 --- a/src/lib/litegraph/src/widgets/BaseWidget.ts +++ b/src/lib/litegraph/src/widgets/BaseWidget.ts @@ -1,5 +1,6 @@ import { t } from '@/i18n' import { drawTextInArea } from '@/lib/litegraph/src/draw' +import { cachedMeasureText } from '@/lib/litegraph/src/utils/textMeasureCache' import { Rectangle } from '@/lib/litegraph/src/infrastructure/Rectangle' import type { Point } from '@/lib/litegraph/src/interfaces' import type { NodeId } from '@/lib/litegraph/src/LGraphNode' @@ -349,8 +350,8 @@ export abstract class BaseWidget // Measure label and value const { displayName, _displayValue } = this - const labelWidth = ctx.measureText(displayName).width - const valueWidth = ctx.measureText(_displayValue).width + const labelWidth = cachedMeasureText(ctx, displayName) + const valueWidth = cachedMeasureText(ctx, _displayValue) const gap = BaseWidget.labelValueGap const x = margin * 2 + leftPadding