From 8bee4bc62cc5b5668ca5dc401d751097f9fabc57 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 26 Jul 2026 22:26:49 -0700 Subject: [PATCH] perf(source-control): share one path collator across the projection (#10850) --- .../source-control-path-sort-benchmark.mjs | 116 ++++++++++++++++++ .../right-sidebar/SourceControl.tsx | 18 +-- .../source-control-status-sort.test.ts | 77 ++++++++++++ .../source-control-status-sort.ts | 6 +- .../right-sidebar/source-control-tree.ts | 4 +- 5 files changed, 201 insertions(+), 20 deletions(-) create mode 100644 config/scripts/source-control-path-sort-benchmark.mjs create mode 100644 src/renderer/src/components/right-sidebar/source-control-status-sort.test.ts diff --git a/config/scripts/source-control-path-sort-benchmark.mjs b/config/scripts/source-control-path-sort-benchmark.mjs new file mode 100644 index 00000000000..039bb4d6f08 --- /dev/null +++ b/config/scripts/source-control-path-sort-benchmark.mjs @@ -0,0 +1,116 @@ +#!/usr/bin/env node +// Benchmark: cost of sorting the Source Control changed-file list. +// +// compareGitStatusEntries called `a.path.localeCompare(b.path, undefined, {numeric:true})`. +// Passing an options object makes each call resolve a fresh ICU collator, so a +// sort paid for one per O(n log n) comparison. The fix hoists a single +// Intl.Collator, which is the idiom TaskPage.tsx already uses for Jira labels. +// +// The sort runs in a useMemo keyed on the entry list, so it re-runs on every git +// refresh that changes the working tree. +// +// Both arms sort the same generated list and their outputs are compared before +// timing, so a comparator that changed the order cannot be reported as a win. +import { execFileSync } from 'node:child_process' +import { performance } from 'node:perf_hooks' +import { fileURLToPath } from 'node:url' + +const ITERATIONS = Number(process.env.ORCA_SC_SORT_BENCH_ITERATIONS ?? '25') +const WARMUP = Number(process.env.ORCA_SC_SORT_BENCH_WARMUP ?? '5') + +for (const [name, value] of [ + ['ORCA_SC_SORT_BENCH_ITERATIONS', ITERATIONS], + ['ORCA_SC_SORT_BENCH_WARMUP', WARMUP] +]) { + if (!Number.isSafeInteger(value) || value <= 0) { + throw new Error(`${name} must be a positive integer, received ${value}`) + } +} + +function conflictRank(entry) { + if (entry.conflictStatus === 'unresolved') { + return 0 + } + if (entry.conflictStatus === 'resolved_locally') { + return 1 + } + return 2 +} + +// Pre-fix: resolves a collator per comparison. +function compareBefore(a, b) { + return ( + conflictRank(a) - conflictRank(b) || a.path.localeCompare(b.path, undefined, { numeric: true }) + ) +} + +// Post-fix: one hoisted collator, mirroring source-control-status-sort.ts. +const collator = new Intl.Collator(undefined, { numeric: true }) +function compareAfter(a, b) { + return conflictRank(a) - conflictRank(b) || collator.compare(a.path, b.path) +} + +// Why real repo paths in git's own order: `git status` emits byte-sorted paths, +// and a shuffled fixture inflates the win — a nearly-sorted array is the case +// this actually has to beat. +const REPO_PATHS = execFileSync('git', ['ls-files'], { + cwd: fileURLToPath(new URL('../..', import.meta.url)), + maxBuffer: 256 * 1024 * 1024 +}) + .toString() + .split('\n') + .filter(Boolean) + +function makeEntries(count) { + const step = Math.max(1, Math.floor(REPO_PATHS.length / count)) + const paths = [] + for (let index = 0; index < REPO_PATHS.length && paths.length < count; index += step) { + paths.push(REPO_PATHS[index]) + } + return paths.map((path, index) => ({ + path, + area: 'unstaged', + status: 'modified', + ...(index % 37 === 0 ? { conflictStatus: 'unresolved' } : {}) + })) +} + +function measure(compare, entries) { + for (let index = 0; index < WARMUP; index += 1) { + ;[...entries].sort(compare) + } + const samples = [] + for (let round = 0; round < 5; round += 1) { + const start = performance.now() + for (let index = 0; index < ITERATIONS; index += 1) { + ;[...entries].sort(compare) + } + samples.push((performance.now() - start) / ITERATIONS) + } + samples.sort((a, b) => a - b) + return samples[2] +} + +const pad = (value, width) => String(value).padStart(width) +console.log('Source Control changed-file sort, per git refresh. Lower is better.') +console.log(`iterations=${ITERATIONS} warmup=${WARMUP} (median of 5 rounds)`) +console.log(`${pad('files', 7)} ${pad('per-call', 11)} ${pad('hoisted', 11)} ${pad('speedup', 9)}`) + +// Sizes from the real distribution over 7,324 non-merge commits on this repo: +// p50 3, p75 7, p90 17, p95 26, p99 63, max 1626. +for (const count of [3, 17, 26, 63, 308, 1000]) { + const entries = makeEntries(count) + const before = [...entries].sort(compareBefore).map((entry) => entry.path) + const after = [...entries].sort(compareAfter).map((entry) => entry.path) + if (before.join('\n') !== after.join('\n')) { + throw new Error(`sort order differs at ${count} files`) + } + const beforeMs = measure(compareBefore, entries) + const afterMs = measure(compareAfter, entries) + console.log( + `${pad(count, 7)} ${pad(`${beforeMs.toFixed(3)} ms`, 11)} ${pad(`${afterMs.toFixed(3)} ms`, 11)} ${pad(`${(beforeMs / afterMs).toFixed(1)}x`, 9)}` + ) +} +console.log( + '\nSizes are the real changed-file distribution over 7,324 non-merge commits on\nthis repo (p50 3, p90 17, p95 26, p99 63), so the top rows are the common case.\nThis times the sort alone; the sort is roughly 85% of the Source Control\nprojection chain, so the end-to-end memo win is smaller than these ratios.' +) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 525c96a3203..d349d789aa0 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -97,6 +97,7 @@ import { namespaceSourceControlTreeDirectoryKeys, type SourceControlTreeNode } from './source-control-tree' +import { compareGitStatusEntries } from './source-control-status-sort' import { collectListSelectionEntries, getSubmoduleExpansionKey, @@ -8213,20 +8214,3 @@ export function ActionButton({ ) } - -function compareGitStatusEntries(a: GitStatusEntry, b: GitStatusEntry): number { - return ( - getConflictSortRank(a) - getConflictSortRank(b) || - a.path.localeCompare(b.path, undefined, { numeric: true }) - ) -} - -function getConflictSortRank(entry: GitStatusEntry): number { - if (entry.conflictStatus === 'unresolved') { - return 0 - } - if (entry.conflictStatus === 'resolved_locally') { - return 1 - } - return 2 -} diff --git a/src/renderer/src/components/right-sidebar/source-control-status-sort.test.ts b/src/renderer/src/components/right-sidebar/source-control-status-sort.test.ts new file mode 100644 index 00000000000..506e49c2949 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control-status-sort.test.ts @@ -0,0 +1,77 @@ +import { describe, expect, it } from 'vitest' +import type { GitStatusEntry } from '../../../../shared/types' +import { compareGitStatusEntries } from './source-control-status-sort' + +function entry(path: string, conflictStatus?: GitStatusEntry['conflictStatus']): GitStatusEntry { + return { + path, + area: 'unstaged', + status: 'modified', + ...(conflictStatus ? { conflictStatus } : {}) + } as GitStatusEntry +} + +// Reference: the pre-change comparator, which resolved a collator per call. +function referenceCompare(a: GitStatusEntry, b: GitStatusEntry): number { + const rank = (value: GitStatusEntry): number => { + if (value.conflictStatus === 'unresolved') { + return 0 + } + if (value.conflictStatus === 'resolved_locally') { + return 1 + } + return 2 + } + return rank(a) - rank(b) || a.path.localeCompare(b.path, undefined, { numeric: true }) +} + +describe('compareGitStatusEntries', () => { + // Why pin the order: the shared collator replaced a per-call localeCompare, so + // the only thing that could regress is the ordering it produces. + it('orders identically to a per-call localeCompare', () => { + const paths = [ + 'src/a.ts', + 'src/A.ts', + 'src/file2.ts', + 'src/file10.ts', + 'src/file1.ts', + 'src/File3.ts', + 'src/nested/deep/z.ts', + 'src/nested/a.ts', + 'README.md', + 'package.json', + 'src/日本語.ts', + 'src/émoji.ts', + 'src/file-2.ts', + 'src/file_2.ts', + 'src/10.ts', + 'src/9.ts' + ] + const entries = paths.map((path) => entry(path)) + const sorted = [...entries].sort(compareGitStatusEntries).map((value) => value.path) + const reference = [...entries].sort(referenceCompare).map((value) => value.path) + expect(sorted).toEqual(reference) + }) + + it('keeps conflicts ahead of clean paths regardless of name', () => { + const entries = [ + entry('z-clean.ts'), + entry('a-resolved.ts', 'resolved_locally'), + entry('m-unresolved.ts', 'unresolved') + ] + expect([...entries].sort(compareGitStatusEntries).map((value) => value.path)).toEqual([ + 'm-unresolved.ts', + 'a-resolved.ts', + 'z-clean.ts' + ]) + }) + + it('sorts numeric path segments naturally', () => { + const entries = ['f10.ts', 'f9.ts', 'f1.ts'].map((path) => entry(path)) + expect([...entries].sort(compareGitStatusEntries).map((value) => value.path)).toEqual([ + 'f1.ts', + 'f9.ts', + 'f10.ts' + ]) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control-status-sort.ts b/src/renderer/src/components/right-sidebar/source-control-status-sort.ts index a2582db74f1..f5ee065bc70 100644 --- a/src/renderer/src/components/right-sidebar/source-control-status-sort.ts +++ b/src/renderer/src/components/right-sidebar/source-control-status-sort.ts @@ -1,9 +1,13 @@ import type { GitStatusEntry } from '../../../../shared/types' +// Why hoisted: localeCompare with an options object resolves a fresh ICU collator +// on every comparison, so a changed-file sort paid for one per O(n log n) step. +export const sourceControlPathCollator = new Intl.Collator(undefined, { numeric: true }) + export function compareGitStatusEntries(a: GitStatusEntry, b: GitStatusEntry): number { return ( getConflictSortRank(a) - getConflictSortRank(b) || - a.path.localeCompare(b.path, undefined, { numeric: true }) + sourceControlPathCollator.compare(a.path, b.path) ) } diff --git a/src/renderer/src/components/right-sidebar/source-control-tree.ts b/src/renderer/src/components/right-sidebar/source-control-tree.ts index 5668e8d2023..6bf94893b07 100644 --- a/src/renderer/src/components/right-sidebar/source-control-tree.ts +++ b/src/renderer/src/components/right-sidebar/source-control-tree.ts @@ -1,7 +1,7 @@ import { normalizeRelativePath } from '@/lib/path' import type { GitStatusEntry, GitStagingArea } from '../../../../shared/types' import { splitPathSegments } from './path-tree' -import { compareGitStatusEntries } from './source-control-status-sort' +import { compareGitStatusEntries, sourceControlPathCollator } from './source-control-status-sort' export type SourceControlTreeArea = Extract // Why: committed branch rows share the same path tree but do not carry @@ -51,7 +51,7 @@ type MutableDirectoryNode(