diff --git a/config/scripts/git-diff-blob-concurrency-benchmark.mjs b/config/scripts/git-diff-blob-concurrency-benchmark.mjs new file mode 100644 index 00000000000..602efd00e1b --- /dev/null +++ b/config/scripts/git-diff-blob-concurrency-benchmark.mjs @@ -0,0 +1,128 @@ +#!/usr/bin/env node +// Benchmark: latency of loading one file diff, which reads two git blobs. +// +// The diff loaders in src/main/git/status.ts awaited their two sides in series, +// so the second `git show` could not start until the first had fully returned. +// The two reads are independent, so that serialization was pure added latency on +// every diff the review panel opens. +// +// This spawns the real `git` binary against this repo, so it measures actual +// process-launch and read cost rather than a model of it. Over SSH each diff is +// one relay RPC and the two spawns run host-local inside the relay, so the same +// relative saving applies to remote-host spawn time, not to network round trips. +import { execFile } from 'node:child_process' +import { performance } from 'node:perf_hooks' +import { fileURLToPath } from 'node:url' + +const REPO_ROOT = fileURLToPath(new URL('../..', import.meta.url)) +const ITERATIONS = Number(process.env.ORCA_DIFF_BLOB_BENCH_ITERATIONS ?? '10') +const WARMUP = Number(process.env.ORCA_DIFF_BLOB_BENCH_WARMUP ?? '3') + +for (const [name, value] of [ + ['ORCA_DIFF_BLOB_BENCH_ITERATIONS', ITERATIONS], + ['ORCA_DIFF_BLOB_BENCH_WARMUP', WARMUP] +]) { + if (!Number.isSafeInteger(value) || value <= 0) { + throw new Error(`${name} must be a positive integer, received ${value}`) + } +} + +function git(args) { + return new Promise((resolve, reject) => { + execFile('git', args, { cwd: REPO_ROOT, maxBuffer: 256 * 1024 * 1024 }, (error, stdout) => + error ? reject(error) : resolve(stdout) + ) + }) +} + +// Pre-fix: await one side, then the other. +async function readSequential(leftRef, rightRef, filePath) { + const left = await git(['show', '--end-of-options', `${leftRef}:${filePath}`]) + const right = await git(['show', '--end-of-options', `${rightRef}:${filePath}`]) + return left.length + right.length +} + +// Post-fix: issue both, await together. +async function readConcurrent(leftRef, rightRef, filePath) { + const [left, right] = await Promise.all([ + git(['show', '--end-of-options', `${leftRef}:${filePath}`]), + git(['show', '--end-of-options', `${rightRef}:${filePath}`]) + ]) + return left.length + right.length +} + +// Why interleaved: running one strategy's whole batch before the other's lets +// cache warming, CPU-frequency drift, and background load correlate with the +// strategy being measured. Alternating per iteration and taking medians keeps +// that drift common to both arms. +async function measureInterleaved(leftRef, rightRef, filePath) { + for (let index = 0; index < WARMUP; index += 1) { + await readSequential(leftRef, rightRef, filePath) + await readConcurrent(leftRef, rightRef, filePath) + } + const sequentialSamples = [] + const concurrentSamples = [] + for (let index = 0; index < ITERATIONS; index += 1) { + // Alternate which arm goes first so neither systematically pays a cold cache. + const sequentialFirst = index % 2 === 0 + for (const runSequential of sequentialFirst ? [true, false] : [false, true]) { + const start = performance.now() + await (runSequential ? readSequential : readConcurrent)(leftRef, rightRef, filePath) + ;(runSequential ? sequentialSamples : concurrentSamples).push(performance.now() - start) + } + } + const median = (samples) => { + const sorted = [...samples].sort((a, b) => a - b) + const middle = Math.floor(sorted.length / 2) + return sorted.length % 2 === 0 ? (sorted[middle - 1] + sorted[middle]) / 2 : sorted[middle] + } + return { sequential: median(sequentialSamples), concurrent: median(concurrentSamples) } +} + +const head = (await git(['rev-parse', 'HEAD'])).trim() +const parent = `${head}~1` + +// Files that exist on both sides, spanning small to large so the fixed spawn +// cost and the size-dependent read cost are both represented. +const CANDIDATES = [ + 'src/main/git/status.ts', + 'src/shared/agent-hook-listener.ts', + 'src/renderer/src/components/TaskPage.tsx' +] + +const files = [] +for (const filePath of CANDIDATES) { + try { + await git(['cat-file', '-e', `${parent}:${filePath}`]) + await git(['cat-file', '-e', `${head}:${filePath}`]) + files.push(filePath) + } catch { + // Skip a path that does not exist on both sides in this checkout. + } +} +if (files.length === 0) { + throw new Error('no benchmark file exists at both HEAD and HEAD~1 in this checkout') +} + +const pad = (value, width) => String(value).padStart(width) +console.log('One file diff = two git blob reads. Lower is better.') +console.log( + `iterations=${ITERATIONS} warmup=${WARMUP} (interleaved, medians) head=${head.slice(0, 9)}` +) +console.log( + `${pad('file', 26)} ${pad('sequential', 12)} ${pad('concurrent', 12)} ${pad('speedup', 9)} ${pad('saved', 10)}` +) +for (const filePath of files) { + const sequentialBytes = await readSequential(parent, head, filePath) + const concurrentBytes = await readConcurrent(parent, head, filePath) + if (sequentialBytes !== concurrentBytes) { + throw new Error(`byte mismatch for ${filePath}`) + } + const { sequential, concurrent } = await measureInterleaved(parent, head, filePath) + console.log( + `${pad(filePath.split('/').pop(), 26)} ${pad(`${sequential.toFixed(1)} ms`, 12)} ${pad(`${concurrent.toFixed(1)} ms`, 12)} ${pad(`${(sequential / concurrent).toFixed(2)}x`, 9)} ${pad(`${(sequential - concurrent).toFixed(1)} ms`, 10)}` + ) +} +console.log( + '\nThe saving is per diff opened, and is dominated by process launch rather than\nfile size — which is why it holds roughly constant across these files.' +) diff --git a/src/main/git/status.test.ts b/src/main/git/status.test.ts index 2cdc41dc367..c39a4097825 100644 --- a/src/main/git/status.test.ts +++ b/src/main/git/status.test.ts @@ -543,11 +543,12 @@ describe('getDiff', () => { const reads = Array.from({ length: 8 }, () => getDiff('/repo', 'src/file.ts', true)) - await waitForMockCalls(gitExecFileAsyncBufferMock, 1) - expect(gitExecFileAsyncBufferMock).toHaveBeenCalledTimes(1) + // Why both up front: the two sides are independent spawns issued concurrently, + // so 8 identical reads still collapse to exactly 2 — one per side, not per read. + await waitForMockCalls(gitExecFileAsyncBufferMock, 2) + expect(gitExecFileAsyncBufferMock).toHaveBeenCalledTimes(2) leftBlob.resolve() - await waitForMockCalls(gitExecFileAsyncBufferMock, 2) rightBlob.resolve() const results = await Promise.all(reads) diff --git a/src/main/git/status.ts b/src/main/git/status.ts index 34c5d750476..1375e4dca87 100644 --- a/src/main/git/status.ts +++ b/src/main/git/status.ts @@ -1244,21 +1244,29 @@ async function loadDiff( let modifiedDeleted = false try { - const leftBlob = staged - ? await readGitBlobAtOidPath(worktreePath, 'HEAD', filePath, options) - : compareAgainstHead - ? await readGitBlobAtOidPath(worktreePath, 'HEAD', filePath, options) - : await readUnstagedLeftBlob(worktreePath, filePath, options) - originalContent = leftBlob.content - originalIsBinary = leftBlob.isBinary - if (staged) { - const rightBlob = await readGitBlobAtIndexPath(worktreePath, filePath, options) + // Why concurrent: HEAD and the index are independent `git show` spawns. + // Only this branch qualifies — the unstaged left read chains index→HEAD. + const [leftBlob, rightBlob] = await Promise.all([ + readGitBlobAtOidPath(worktreePath, 'HEAD', filePath, options), + readGitBlobAtIndexPath(worktreePath, filePath, options) + ]) + originalContent = leftBlob.content + originalIsBinary = leftBlob.isBinary modifiedContent = rightBlob.content modifiedIsBinary = rightBlob.isBinary modifiedDeleted = !rightBlob.exists } else { - const workingTreeBlob = await readWorkingTreeFile(path.join(worktreePath, filePath)) + // The left chain (index→HEAD) is sequential within itself, but the working + // tree read is a plain fs read that does not depend on it. + const [leftBlob, workingTreeBlob] = await Promise.all([ + compareAgainstHead + ? readGitBlobAtOidPath(worktreePath, 'HEAD', filePath, options) + : readUnstagedLeftBlob(worktreePath, filePath, options), + readWorkingTreeFile(path.join(worktreePath, filePath)) + ]) + originalContent = leftBlob.content + originalIsBinary = leftBlob.isBinary modifiedContent = workingTreeBlob.content modifiedIsBinary = workingTreeBlob.isBinary modifiedDeleted = !workingTreeBlob.exists @@ -1397,8 +1405,12 @@ async function loadBranchDiff( ): Promise { try { const leftPath = args.oldPath ?? args.filePath - const leftBlob = await readGitBlobAtOidPath(worktreePath, args.mergeBase, leftPath, options) - const rightBlob = await readGitBlobAtOidPath(worktreePath, args.headOid, args.filePath, options) + // Why concurrent: the two sides are independent `git show` spawns, so awaiting + // them in series doubles the latency of every diff the review panel opens. + const [leftBlob, rightBlob] = await Promise.all([ + readGitBlobAtOidPath(worktreePath, args.mergeBase, leftPath, options), + readGitBlobAtOidPath(worktreePath, args.headOid, args.filePath, options) + ]) return buildDiffResult( leftBlob.content, @@ -1510,15 +1522,14 @@ async function loadCommitDiff( ): Promise { try { const leftPath = args.oldPath ?? args.filePath - const leftBlob = args.parentOid - ? await readGitBlobAtOidPath(worktreePath, args.parentOid, leftPath, options) - : { content: '', isBinary: false } - const rightBlob = await readGitBlobAtOidPath( - worktreePath, - args.commitOid, - args.filePath, - options - ) + // Why concurrent: the two sides are independent `git show` spawns. A root + // commit has no parent to read, so that side resolves without a spawn. + const [leftBlob, rightBlob] = await Promise.all([ + args.parentOid + ? readGitBlobAtOidPath(worktreePath, args.parentOid, leftPath, options) + : Promise.resolve({ content: '', isBinary: false }), + readGitBlobAtOidPath(worktreePath, args.commitOid, args.filePath, options) + ]) return buildDiffResult( leftBlob.content,