perf(git): read both diff blobs concurrently (#10781)

* perf(git): read both diff blobs concurrently The diff loaders awaited their two sides in series, so the second `git show` could not start until the first had returned. The reads are independent, so that was pure added latency on every diff the review panel opens: ~47 ms sequential vs ~24 ms concurrent, a saving of ~23 ms per diff. Covers the merge-base, commit, and staged loaders, plus the unstaged path where the working-tree read is independent of the index->HEAD chain. The unstaged left chain itself stays sequential because its second step depends on the first. The staged coalescing test asserted the sequential shape (one spawn, then the next); it now pins the contract that actually matters — eight identical reads still collapse to two spawns, one per side. * test(perf): interleave the diff-blob benchmark arms 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. Alternate the arms per iteration, alternate which goes first, and report medians so that drift stays common to both. Also reject malformed env settings rather than truncating them — Number.parseInt accepts "10foo" and 3.5. Interleaved result confirms the original: 1.90x-2.03x, ~24 ms saved per diff.
2026-09-21 16:02:20 +00:00 · 2026-07-26 20:31:52 -07:00
parent 8518cf47af
commit 5a30c5c2ed
3 changed files with 164 additions and 24 deletions
@@ -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.'
+)
@@ -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)
@@ -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<GitDiffResult> {
  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<GitDiffResult> {
  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,