diff --git a/packages/app/e2e/performance/patch-groups/README.md b/packages/app/e2e/performance/patch-groups/README.md new file mode 100644 index 00000000000..b5530e5875d --- /dev/null +++ b/packages/app/e2e/performance/patch-groups/README.md @@ -0,0 +1,41 @@ +# Patch Group Benchmark + +This manual benchmark mounts the production `CurrentFileToolGroup` and `File` +components with completed edit results. A separate case mounts `ToolDisplay` +with a patch result. It uses four real Core tool source files, with deterministic +identifier renames, rather than repeated filler. It does not connect to a server. + +From `packages/app`, set `PATCH_BUILD_DIR` and `PATCH_RESULTS_DIR` to external +artifact directories, then run: + +```sh +bun x vite build --config e2e/performance/patch-groups/vite.config.ts +bun x playwright test --config e2e/performance/patch-groups/playwright.config.ts --repeat-each=20 +``` + +Run under the shared exclusive gate when collecting measurements on a shared +machine. The Playwright-owned static server uses `PATCH_PORT` (default 4317), +refuses to reuse an existing server, and shuts down after the run. + +Each fresh browser context measures a cold collapsed mount, a warm remount, +and opening `edit.ts` through its real accordion. Mount timing covers synchronous +component construction through layout. Expansion timing starts at the click and +ends at the production file renderer's `onRendered` callback. Assertions check +the exact file count, collapsed state, and completed file rendering. Results +include payload bytes, source bytes, file/tool counts, and supporting warm +`patchFileGroups` timings with and without reading views. No timing thresholds +are enforced. This is a browser component workload, not a full desktop memory test. + +Freeze the build before changing production code. Use the same fixture, browser, +viewport, sample count, and completion checks for both revisions. + +`PATCH_REVISION=` loads the grouping module and tool renderer from that +revision at build time without changing the worktree. This is useful when fixing +the harness after freezing a baseline. All other production sources must match +between revisions; this switch only covers those two measured modules. + +For a separate diagnostic build, set `PATCH_COUNTERS=1`. Its build-only transform +counts grouping, normalization, reconstruction, and line-diff calls with User +Timing marks. Do not mix instrumented results with clean timings. Set +`OPENCODE_PERFORMANCE_TRACE_DIR` for the existing Chrome trace collector, and +`PATCH_SCREENSHOTS=1` for collapsed/expanded screenshots after measurement. diff --git a/packages/app/e2e/performance/patch-groups/fixture.tsx b/packages/app/e2e/performance/patch-groups/fixture.tsx new file mode 100644 index 00000000000..32cc31fc6ae --- /dev/null +++ b/packages/app/e2e/performance/patch-groups/fixture.tsx @@ -0,0 +1,143 @@ +/// + +import { render } from "solid-js/web" +import { Show } from "solid-js" +import { createStore } from "solid-js/store" +import { ThemeProvider } from "@opencode-ai/ui/theme" +import { CurrentSessionProviders } from "../../../../session-ui/src/storybook/current-session-story" +import { emptySessionDocument } from "../../../../session-ui/src/storybook/current-session-fixtures" +import { CurrentFileToolGroup, ToolDisplay } from "../../../../session-ui/src/tools/tool-renderer" +import { patchFileGroups } from "../../../../session-ui/src/components/apply-patch-file" +import type { SessionMessageAssistantTool } from "@opencode-ai/client/promise" +import { createTwoFilesPatch, diffLines } from "diff" +import edit from "../../../../core/src/tool/plugin/edit.ts?raw" +import patch from "../../../../core/src/tool/plugin/patch.ts?raw" +import read from "../../../../core/src/tool/plugin/read.ts?raw" +import shell from "../../../../core/src/tool/plugin/shell.ts?raw" +import "../../../src/index.css" + +const scenario = new URLSearchParams(location.search).get("scenario") ?? "complete" +const sources = [edit, patch, read, shell].map((text) => text.replaceAll("\r\n", "\n")) +const names = ["edit", "patch", "read", "shell"] +const changed = (text: string) => text.replaceAll(/\bcontext\b/g, "invocation") +const entry = (index: number, before: string, after: string) => ({ + file: `src/tool/plugin/${names[index]}.ts`, + patch: createTwoFilesPatch(names[index], names[index], before, after, "", "", { + context: scenario === "partial" ? 3 : Infinity, + }), + ...diffLines(before, after).reduce( + (counts, item) => ({ + additions: counts.additions + (item.added ? item.count : 0), + deletions: counts.deletions + (item.removed ? item.count : 0), + }), + { additions: 0, deletions: 0 }, + ), + status: "modified" as const, +}) +const files = + scenario === "multi" + ? sources.map((text, index) => entry(index, text, changed(text))) + : [ + entry(0, sources[0], changed(sources[0])), + ...(scenario === "chained" + ? [entry(0, changed(sources[0]), changed(sources[0]).replaceAll(/\binput\b/g, "parameters"))] + : []), + ] +const tools: SessionMessageAssistantTool[] = files.map((file, index) => ({ + id: `fixture-edit-${index}`, + type: "tool", + name: "edit", + state: { + status: "completed", + input: { path: file.file, oldString: "context", newString: "invocation", replaceAll: true }, + metadata: { files: [file] }, + content: [{ type: "text", text: `Edited ${file.file}` }], + }, + time: { created: 1, ran: 2, completed: 3 }, +})) + +declare global { + interface Window { + patchBenchmark: { + payloadBytes: number + sourceBytes: number + files: number + tools: number + grouping: (expanded: boolean) => { ms: number; groups: number; views: number } + } + } +} +window.patchBenchmark = { + payloadBytes: new TextEncoder().encode(JSON.stringify(tools)).length, + sourceBytes: new TextEncoder().encode(sources.slice(0, scenario === "multi" ? 4 : 1).join("")).length, + files: new Set(files.map((file) => file.file)).size, + tools: tools.length, + grouping(expanded) { + const start = performance.now() + const groups = patchFileGroups(files) + const views = expanded ? groups.reduce((count, file) => count + file.views.length, 0) : 0 + return { ms: performance.now() - start, groups: groups.length, views } + }, +} + +function Fixture() { + const [state, setState] = createStore({ mounted: false, duration: 0, rendered: 0 }) + let start = 0 + return ( + +
+ + + {state.duration} + {state.rendered} +
+ + + setState("rendered", performance.now() - start)} + /> + } + > + setState("rendered", performance.now() - start)} + /> + + + +
+
+ + ) +} +render(() => , document.getElementById("root")!) diff --git a/packages/app/e2e/performance/patch-groups/index.html b/packages/app/e2e/performance/patch-groups/index.html new file mode 100644 index 00000000000..9beaa85afd5 --- /dev/null +++ b/packages/app/e2e/performance/patch-groups/index.html @@ -0,0 +1,11 @@ + + + + + Patch groups benchmark + + +
+ + + diff --git a/packages/app/e2e/performance/patch-groups/patch-groups.bench.ts b/packages/app/e2e/performance/patch-groups/patch-groups.bench.ts new file mode 100644 index 00000000000..5c372df6d05 --- /dev/null +++ b/packages/app/e2e/performance/patch-groups/patch-groups.bench.ts @@ -0,0 +1,66 @@ +import { benchmark, expect } from "../benchmark" + +for (const scenario of ["complete", "partial", "chained", "multi", "direct"]) { + benchmark(`patch groups ${scenario}`, async ({ page, report }, info) => { + await page.goto(`/?scenario=${scenario}`) + await expect(page.getByRole("button", { name: "Mount tools", exact: true })).toBeEnabled() + await page.evaluate(() => document.fonts.ready) + expect(await page.evaluate(() => document.fonts.check('13px "Inter"'))).toBe(true) + const shape = await page.evaluate(() => { + const { grouping, ...shape } = window.patchBenchmark + performance.clearMarks() + return shape + }) + const mount = async () => { + await page.getByRole("button", { name: "Mount tools", exact: true }).click() + await expect(page.locator('[data-slot="apply-patch-filename"]')).toHaveCount(shape.files) + await expect(page.locator('[data-component="file"]')).toHaveCount(0) + return Number(await page.getByTestId("mount-ms").textContent()) + } + const cold = await mount() + const counters = await page.evaluate(() => + Object.fromEntries( + ["patchFileGroups", "normalize", "completePatchContents", "diffLines"].map((name) => [ + name, + performance.getEntriesByName(`patch-counter:${name}`).length, + ]), + ), + ) + await page.getByRole("button", { name: "Unmount tools", exact: true }).click() + await expect(page.locator('[data-component="apply-patch-tool"]')).toHaveCount(0) + await page.evaluate(() => performance.clearMarks()) + const warm = await mount() + const warmCounters = await page.evaluate(() => + Object.fromEntries( + ["patchFileGroups", "normalize", "completePatchContents", "diffLines"].map((name) => [ + name, + performance.getEntriesByName(`patch-counter:${name}`).length, + ]), + ), + ) + const file = page.locator('[data-scope="apply-patch"] button').filter({ hasText: "edit.ts" }) + await expect(file).toHaveAttribute("aria-expanded", "false") + await file.click() + await expect(file).toHaveAttribute("aria-expanded", "true") + await expect(page.getByTestId("rendered")).not.toHaveText("0") + await expect(page.locator('[data-component="file"]')).toBeVisible() + const expansion = Number(await page.getByTestId("rendered").textContent()) + const grouping = await page.evaluate(() => ({ + collapsed: window.patchBenchmark.grouping(false), + expanded: window.patchBenchmark.grouping(true), + })) + expect(grouping.collapsed.groups).toBe(shape.files) + report( + { cold, warm, expansion, grouping, counters, warmCounters }, + { scenario, ...shape, scope: "production tool components" }, + ) + if (process.env.PATCH_SCREENSHOTS === "1") { + await page.screenshot({ path: info.outputPath(`${scenario}-expanded.png`) }) + await file.click() + await expect(file).toHaveAttribute("aria-expanded", "false") + await page + .locator('[data-component="apply-patch-tool"]') + .screenshot({ path: info.outputPath(`${scenario}-collapsed.png`) }) + } + }) +} diff --git a/packages/app/e2e/performance/patch-groups/playwright.config.ts b/packages/app/e2e/performance/patch-groups/playwright.config.ts new file mode 100644 index 00000000000..63b5f0a2fcf --- /dev/null +++ b/packages/app/e2e/performance/patch-groups/playwright.config.ts @@ -0,0 +1,18 @@ +import { defineConfig } from "@playwright/test" + +const baseURL = `http://127.0.0.1:${process.env.PATCH_PORT ?? 4317}` +export default defineConfig({ + testDir: ".", + testMatch: "*.bench.ts", + workers: 1, + retries: 0, + timeout: 60_000, + outputDir: process.env.PATCH_RESULTS_DIR, + reporter: "line", + use: { baseURL, viewport: { width: 1366, height: 768 }, colorScheme: "light" }, + webServer: { + command: "bun serve.ts", + url: baseURL, + reuseExistingServer: false, + }, +}) diff --git a/packages/app/e2e/performance/patch-groups/serve.ts b/packages/app/e2e/performance/patch-groups/serve.ts new file mode 100644 index 00000000000..c2b22842b0e --- /dev/null +++ b/packages/app/e2e/performance/patch-groups/serve.ts @@ -0,0 +1,13 @@ +import path from "node:path" + +const directory = process.env.PATCH_BUILD_DIR +if (!directory) throw new Error("PATCH_BUILD_DIR is required") +Bun.serve({ + hostname: "127.0.0.1", + port: Number(process.env.PATCH_PORT ?? 4317), + async fetch(request) { + const pathname = new URL(request.url).pathname + const file = Bun.file(path.join(directory, pathname === "/" ? "index.html" : pathname)) + return (await file.exists()) ? new Response(file) : new Response("Not found", { status: 404 }) + }, +}) diff --git a/packages/app/e2e/performance/patch-groups/vite.config.ts b/packages/app/e2e/performance/patch-groups/vite.config.ts new file mode 100644 index 00000000000..61e002dd528 --- /dev/null +++ b/packages/app/e2e/performance/patch-groups/vite.config.ts @@ -0,0 +1,51 @@ +import { defineConfig } from "vite" +import solid from "vite-plugin-solid" +import tailwindcss from "@tailwindcss/vite" +import { fileURLToPath } from "node:url" +import { execFileSync } from "node:child_process" +import path from "node:path" + +export default defineConfig({ + root: fileURLToPath(new URL(".", import.meta.url)), + publicDir: fileURLToPath(new URL("../../../public", import.meta.url)), + plugins: [ + solid(), + tailwindcss(), + { + name: "patch-group-counters", + enforce: "pre", + load(id) { + if (!process.env.PATCH_REVISION) return + const root = fileURLToPath(new URL("../../../../..", import.meta.url)) + const file = path.relative(root, id).replaceAll("\\", "/") + if ( + ![ + "packages/session-ui/src/components/apply-patch-file.ts", + "packages/session-ui/src/tools/tool-renderer.tsx", + ].includes(file) + ) + return + return execFileSync("git", ["show", `${process.env.PATCH_REVISION}:${file}`], { cwd: root, encoding: "utf8" }) + }, + transform(code, id) { + if (process.env.PATCH_COUNTERS !== "1") return + const functions = id.replaceAll("\\", "/").endsWith("/apply-patch-file.ts") + ? ["patchFileGroups"] + : id.replaceAll("\\", "/").endsWith("/session-diff.ts") + ? ["normalize", "completePatchContents"] + : id.replaceAll("\\", "/").endsWith("/diff/line.js") + ? ["diffLines"] + : [] + for (const name of functions) { + const pattern = new RegExp(`(export function ${name}\\([^)]*\\)[^{]*\\{)`) + if (!pattern.test(code)) throw new Error(`Missing instrumented function ${name} in ${id}`) + code = code.replace(pattern, `$1 performance.mark("patch-counter:${name}");`) + } + return functions.length ? { code, map: null } : undefined + }, + }, + ], + resolve: { dedupe: ["solid-js", "@solidjs/meta"] }, + worker: { format: "es" }, + build: { outDir: process.env.PATCH_BUILD_DIR, emptyOutDir: true, sourcemap: true }, +}) diff --git a/packages/session-ui/src/components/apply-patch-file.test.ts b/packages/session-ui/src/components/apply-patch-file.test.ts index ba2bef6c57d..f203a6aee77 100644 --- a/packages/session-ui/src/components/apply-patch-file.test.ts +++ b/packages/session-ui/src/components/apply-patch-file.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "bun:test" import { createTwoFilesPatch } from "diff" import { patchFile, patchFileGroups, patchFiles } from "./apply-patch-file" +import { text } from "./session-diff" describe("apply patch files", () => { test("parses current file diffs", () => { @@ -80,4 +81,84 @@ describe("apply patch files", () => { expect(groups).toHaveLength(1) expect(groups[0]?.views).toHaveLength(2) }) + + test.each(["\n", "\r\n"])("preserves complete chained contents with %j line endings", (newline) => { + const before = `const count = 1${newline}export { count }` + const middle = `const count = 2${newline}export { count }` + const after = `const count = 3${newline}export { count }` + const groups = patchFileGroups( + [before, middle].map((value, index) => ({ + file: "count.ts", + patch: createTwoFilesPatch("count.ts", "count.ts", value, index === 0 ? middle : after, "", "", { + context: Infinity, + }), + status: "modified", + additions: 1, + deletions: 1, + })), + ) + + expect(groups).toHaveLength(1) + expect(groups[0]).toMatchObject({ type: "update", additions: 1, deletions: 1 }) + expect(groups[0]!.views).toHaveLength(1) + expect(text(groups[0]!.views[0]!, "deletions")).toBe(before) + expect(text(groups[0]!.views[0]!, "additions")).toBe(after) + expect(groups[0]!.views).toBe(groups[0]!.views) + }) + + test("uses net counts for complete patches instead of producer counts", () => { + const groups = patchFileGroups([ + { + file: "count.ts", + patch: createTwoFilesPatch("count.ts", "count.ts", "one\n", "two\n", "", "", { context: Infinity }), + status: "modified", + additions: 4, + deletions: 5, + }, + ]) + expect(groups[0]).toMatchObject({ additions: 1, deletions: 1 }) + expect(groups[0]!.views[0]).toMatchObject({ additions: 1, deletions: 1 }) + }) + + test("keeps disconnected complete patches separate and preserves file order", () => { + const groups = patchFileGroups( + [ + ["b.ts", "one\n", "two\n"], + ["a.ts", "first\n", "second\n"], + ["b.ts", "three\n", "four\n"], + ].map(([file, before, after]) => ({ + file, + patch: createTwoFilesPatch(file!, file!, before!, after!, "", "", { context: Infinity }), + status: "modified", + additions: 1, + deletions: 1, + })), + ) + expect(groups.map((group) => group.path)).toEqual(["b.ts", "a.ts"]) + expect(groups[0]).toMatchObject({ additions: 2, deletions: 2 }) + expect(groups[0]!.views.map((view) => text(view, "additions"))).toEqual(["two\n", "four\n"]) + }) + + test.each([ + ["", "created\n", "", "added", "deleted", "delete", 0, 0], + ["original\n", "changed\n", "original\n", "modified", "modified", "update", 0, 0], + ["", "created\n", "changed\n", "added", "modified", "add", 1, 0], + ])("preserves chain status and cancellation %#", (before, middle, after, first, last, type, additions, deletions) => { + const groups = patchFileGroups( + [ + { before, after: middle, status: first }, + { before: middle, after, status: last }, + ].map((value) => ({ + file: "chain.ts", + patch: createTwoFilesPatch("chain.ts", "chain.ts", value.before, value.after, "", "", { context: Infinity }), + status: value.status, + additions: 1, + deletions: 1, + })), + ) + expect(groups[0]).toMatchObject({ type, additions, deletions }) + expect(groups[0]!.views).toHaveLength(1) + expect(text(groups[0]!.views[0]!, "deletions")).toBe(before) + expect(text(groups[0]!.views[0]!, "additions")).toBe(after) + }) }) diff --git a/packages/session-ui/src/components/apply-patch-file.ts b/packages/session-ui/src/components/apply-patch-file.ts index ffe12999fa3..dc3da09406e 100644 --- a/packages/session-ui/src/components/apply-patch-file.ts +++ b/packages/session-ui/src/components/apply-patch-file.ts @@ -1,5 +1,4 @@ import type { FileDiffInfo } from "@opencode-ai/client/promise" -import { diffLines } from "diff" import { completePatchContents, normalize, type ViewDiff } from "./session-diff" type Kind = "add" | "update" | "delete" @@ -28,12 +27,15 @@ export function changedFileDiff(value: unknown): value is FileDiffInfo { export function patchFile(value: unknown): ApplyPatchFile | undefined { if (!changedFileDiff(value)) return + let view: ViewDiff | undefined return { path: value.file, type: value.status === "added" ? "add" : value.status === "deleted" ? "delete" : "update", additions: value.additions, deletions: value.deletions, - view: normalize(value), + get view() { + return (view ??= normalize(value)) + }, contents: completePatchContents(value.patch), } } @@ -67,12 +69,22 @@ export function patchFileGroups(value: unknown): ApplyPatchFileGroup[] { } } - const before = first.contents!.before - const after = last.contents!.after - const counts = diffLines(before, after).reduce( - (result, item) => ({ - additions: result.additions + (item.added ? (item.count ?? 0) : 0), - deletions: result.deletions + (item.removed ? (item.count ?? 0) : 0), + const view = + files.length === 1 + ? first.view + : normalize({ + file: path, + before: first.contents!.before, + after: last.contents!.after, + status: type === "add" ? "added" : type === "delete" ? "deleted" : "modified", + additions: 0, + deletions: 0, + }) + // Parsed hunks already contain net change counts, excluding unchanged context. + const counts = view.fileDiff.hunks.reduce( + (result, hunk) => ({ + additions: result.additions + hunk.additionLines, + deletions: result.deletions + hunk.deletionLines, }), { additions: 0, deletions: 0 }, ) @@ -80,15 +92,7 @@ export function patchFileGroups(value: unknown): ApplyPatchFileGroup[] { path, type, ...counts, - views: [ - normalize({ - file: path, - before, - after, - status: type === "add" ? "added" : type === "delete" ? "deleted" : "modified", - ...counts, - }), - ], + views: [{ ...view, ...counts }], } }) } diff --git a/packages/session-ui/src/tools/tool-renderer.tsx b/packages/session-ui/src/tools/tool-renderer.tsx index 7255b10dbf0..02e8021cd7b 100644 --- a/packages/session-ui/src/tools/tool-renderer.tsx +++ b/packages/session-ui/src/tools/tool-renderer.tsx @@ -1027,7 +1027,9 @@ function toolErrorSubtitle(props: ToolProps, i18n: UiI18n) { if (props.tool === "websearch") return text(props.input.query) if (props.tool === "skill") return skillToolName(props.input, props.metadata) if (props.tool === "patch") { - const count = patchFileGroups(props.metadata.files).length + const count = new Set( + Array.isArray(props.metadata.files) ? props.metadata.files.filter(changedFileDiff).map((file) => file.file) : [], + ).size if (count === 0) return undefined return `${count} ${i18n.plural("ui.common.file", count)}` }