From 7ed86a98ae97d2e64728cd307f2d546374d52e97 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 20:41:17 -0700 Subject: [PATCH] perf(ipc): index worktree owners instead of rescanning the repo list per lookup (#18416) Two hot lookups rescanned a whole table once per repo. `getLocalRepoForRegisteredWorktree` (59 IPC call sites, including Quick Open keystrokes and every File Explorer expand) walked the entire worktree-meta table once per repo. One pass now collects the owning repo ids, built lazily so a repo whose own path matches still never touches the table. `createRepoRowExecutionHostLookup` re-filtered the repo array on every `byId` / `byHost` call. Rows are grouped into a Map once at construction, preserving repo-list order so `rows[0]` still picks the same owner. --- .../local-worktree-runtime-options.test.ts | 142 ++++++++++++++++++ .../ipc/local-worktree-runtime-options.ts | 20 ++- ...worktree-execution-host-resolution.test.ts | 67 ++++++++- .../worktree-execution-host-resolution.ts | 29 +++- 4 files changed, 244 insertions(+), 14 deletions(-) create mode 100644 src/main/ipc/local-worktree-runtime-options.test.ts diff --git a/src/main/ipc/local-worktree-runtime-options.test.ts b/src/main/ipc/local-worktree-runtime-options.test.ts new file mode 100644 index 00000000000..f7a1b0430e1 --- /dev/null +++ b/src/main/ipc/local-worktree-runtime-options.test.ts @@ -0,0 +1,142 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { WORKTREE_ID_SEPARATOR, type ParsedWorktreeId } from '../../shared/worktree/id' +import type * as WorktreeIdModule from '../../shared/worktree/id' + +const counter = vi.hoisted(() => ({ splitCalls: 0 })) + +// Why: `splitWorktreeId` (and the `Object.keys` snapshot around it) is the per-row work the repo +// loop used to repeat once per repo. Counting it makes the O(repos x rows) regression observable. +vi.mock('../../shared/worktree/id', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + splitWorktreeId: (worktreeId: string): ParsedWorktreeId | null => { + counter.splitCalls += 1 + return actual.splitWorktreeId(worktreeId) + } + } +}) + +const { getLocalRepoForRegisteredWorktree } = await import('./local-worktree-runtime-options') + +type TestRepo = { id: string; path: string; connectionId?: string } + +const makeStore = ( + repos: readonly TestRepo[], + worktreeIds: readonly string[] +): { store: never; metaScans: () => number } => { + let metaScans = 0 + const meta = Object.fromEntries(worktreeIds.map((id) => [id, {}])) + const store = { + getRepos: () => repos, + getAllWorktreeMeta: () => { + metaScans += 1 + return meta + } + } + return { store: store as never, metaScans: () => metaScans } +} + +const worktreeId = (repoId: string, path: string): string => + `${repoId}${WORKTREE_ID_SEPARATOR}${path}` + +beforeEach(() => { + counter.splitCalls = 0 +}) + +describe('getLocalRepoForRegisteredWorktree', () => { + it('walks the worktree meta table once, not once per repo', () => { + // Worst case: the owning repo is last, so every earlier repo used to force a full rescan. + const repoCount = 10 + const rowCount = 200 + const repos = Array.from({ length: repoCount }, (_, i) => ({ + id: `repo-${i}`, + path: `/repos/repo-${i}` + })) + const target = '/repos/repo-9/wt-last' + const worktreeIds = Array.from({ length: rowCount }, (_, i) => + worktreeId(`repo-${i % repoCount}`, `/repos/wt-${i}`) + ) + worktreeIds[rowCount - 1] = worktreeId(`repo-${repoCount - 1}`, target) + const { store, metaScans } = makeStore(repos, worktreeIds) + + expect(getLocalRepoForRegisteredWorktree(store, target, target)?.id).toBe('repo-9') + expect(metaScans()).toBe(1) + expect(counter.splitCalls).toBe(rowCount) + }) + + it('never touches the meta table when a repo path matches directly', () => { + const { store, metaScans } = makeStore([{ id: 'repo-a', path: '/repos/a' }], []) + expect(getLocalRepoForRegisteredWorktree(store, '/repos/a', '/repos/a')?.id).toBe('repo-a') + expect(metaScans()).toBe(0) + }) + + describe('equivalence with the per-repo scan', () => { + const repos: TestRepo[] = [ + { id: 'first', path: '/repos/first' }, + { id: 'middle', path: '/repos/middle' }, + { id: 'last', path: '/repos/last' } + ] + + it('finds a worktree owned by the first repo', () => { + const { store } = makeStore(repos, [worktreeId('first', '/wt/one')]) + expect(getLocalRepoForRegisteredWorktree(store, '/wt/one', '/wt/one')?.id).toBe('first') + }) + + it('finds a worktree owned by the last repo', () => { + const { store } = makeStore(repos, [worktreeId('last', '/wt/one')]) + expect(getLocalRepoForRegisteredWorktree(store, '/wt/one', '/wt/one')?.id).toBe('last') + }) + + it('returns undefined when no repo owns the worktree', () => { + const { store } = makeStore(repos, [worktreeId('other', '/wt/elsewhere')]) + expect(getLocalRepoForRegisteredWorktree(store, '/wt/one', '/wt/one')).toBeUndefined() + }) + + it('keeps getRepos precedence when two repos both own the path', () => { + const { store } = makeStore(repos, [ + worktreeId('last', '/wt/shared'), + worktreeId('middle', '/wt/shared') + ]) + // getRepos order decides, not the meta table's insertion order. + expect(getLocalRepoForRegisteredWorktree(store, '/wt/shared', '/wt/shared')?.id).toBe( + 'middle' + ) + }) + + it('excludes an SSH repo even when it owns the registered worktree', () => { + const { store } = makeStore( + [{ id: 'remote', path: '/repos/remote', connectionId: 'm4air' }, ...repos], + [worktreeId('remote', '/wt/one'), worktreeId('middle', '/wt/one')] + ) + expect(getLocalRepoForRegisteredWorktree(store, '/wt/one', '/wt/one')?.id).toBe('middle') + + const onlyRemote = makeStore( + [{ id: 'remote', path: '/repos/remote', connectionId: 'm4air' }], + [worktreeId('remote', '/wt/one')] + ) + expect( + getLocalRepoForRegisteredWorktree(onlyRemote.store, '/wt/one', '/wt/one') + ).toBeUndefined() + }) + + it('matches the resolved path spelling as well as the raw one', () => { + const { store } = makeStore(repos, [worktreeId('middle', '/wt/one')]) + expect(getLocalRepoForRegisteredWorktree(store, '/wt/other', '/wt/one')?.id).toBe('middle') + }) + + it('returns undefined for a folder workspace that is not a registered worktree', () => { + const { store } = makeStore(repos, [worktreeId('middle', '/wt/one')]) + expect( + getLocalRepoForRegisteredWorktree(store, '/folders/notes', '/folders/notes') + ).toBeUndefined() + }) + + it('tolerates a store without getRepos or getAllWorktreeMeta', () => { + expect(getLocalRepoForRegisteredWorktree({} as never, '/wt/one', '/wt/one')).toBeUndefined() + expect( + getLocalRepoForRegisteredWorktree({ getRepos: () => repos } as never, '/wt/one', '/wt/one') + ).toBeUndefined() + }) + }) +}) diff --git a/src/main/ipc/local-worktree-runtime-options.ts b/src/main/ipc/local-worktree-runtime-options.ts index d0d1a67e3cd..07bb9b41c83 100644 --- a/src/main/ipc/local-worktree-runtime-options.ts +++ b/src/main/ipc/local-worktree-runtime-options.ts @@ -19,20 +19,21 @@ function getCandidateLocalWorktreePaths( return new Set([worktreePath, resolvedWorktreePath].map(comparableLocalPath)) } -function hasRegisteredWorktreeMetaForRepo( +/** Repos owning a registered worktree at one of `candidatePaths`, in one pass over the meta table. */ +function collectRepoIdsWithRegisteredWorktreeMeta( store: Store, - repoId: string, candidatePaths: Set -): boolean { +): Set { const worktreeMeta = typeof store.getAllWorktreeMeta === 'function' ? store.getAllWorktreeMeta() : {} + const repoIds = new Set() for (const worktreeId of Object.keys(worktreeMeta)) { const parsed = splitWorktreeId(worktreeId) - if (parsed?.repoId === repoId && candidatePaths.has(comparableLocalPath(parsed.worktreePath))) { - return true + if (parsed && candidatePaths.has(comparableLocalPath(parsed.worktreePath))) { + repoIds.add(parsed.repoId) } } - return false + return repoIds } export function getLocalRepoForRegisteredWorktree( @@ -45,13 +46,18 @@ export function getLocalRepoForRegisteredWorktree( } const candidatePaths = getCandidateLocalWorktreePaths(worktreePath, resolvedWorktreePath) + // Built at most once, and only when a repo actually needs it, so the meta table is never + // rescanned per repo — 59 IPC call sites hit this, some per keystroke. + let repoIdsWithMeta: Set | undefined return store .getRepos() .find( (repo) => !repo.connectionId && (candidatePaths.has(comparableLocalPath(repo.path)) || - hasRegisteredWorktreeMetaForRepo(store, repo.id, candidatePaths)) + (repoIdsWithMeta ??= collectRepoIdsWithRegisteredWorktreeMeta(store, candidatePaths)).has( + repo.id + )) ) } diff --git a/src/shared/worktree-execution-host-resolution.test.ts b/src/shared/worktree-execution-host-resolution.test.ts index 70ee6d304b6..b82cea476eb 100644 --- a/src/shared/worktree-execution-host-resolution.test.ts +++ b/src/shared/worktree-execution-host-resolution.test.ts @@ -1,7 +1,8 @@ import { describe, expect, it } from 'vitest' import { createRepoRowExecutionHostLookup, - resolveWorktreeExecutionHost + resolveWorktreeExecutionHost, + type ExecutionHostOwnerRow } from './worktree-execution-host-resolution' // Why (#11163, #17799): main's terminal launch scope and the renderer's owner index both answer @@ -165,3 +166,67 @@ describe('resolveWorktreeExecutionHost', () => { }) }) }) + +describe('createRepoRowExecutionHostLookup', () => { + /** Rows whose `id` reads are counted, so a rescan of the repo list is observable. */ + const countingRepos = ( + rows: readonly ExecutionHostOwnerRow[] + ): { repos: ExecutionHostOwnerRow[]; idReads: () => number } => { + let idReads = 0 + const repos = rows.map(({ id, ...rest }) => ({ + ...rest, + get id(): string { + idReads += 1 + return id + } + })) + return { repos, idReads: () => idReads } + } + + it('scans the repo list once for the factory, never again per lookup', () => { + const { repos, idReads } = countingRepos([ + { id: 'a' }, + { id: 'b', connectionId: 'm4air' }, + { id: 'c' } + ]) + const lookup = createRepoRowExecutionHostLookup(repos) + // One grouping pass over the list — a Map get plus a set per row — and then never again. + const afterBuild = idReads() + expect(afterBuild).toBeLessThanOrEqual(repos.length * 2) + + for (let i = 0; i < 50; i++) { + lookup.byId('a') + lookup.byId('missing') + lookup.byHost('b', 'ssh:m4air') + } + expect(idReads()).toBe(afterBuild) + }) + + it('answers missing, ambiguous and resolved exactly as a per-call scan would', () => { + expect(createRepoRowExecutionHostLookup([]).byId('r')).toEqual({ kind: 'missing' }) + + const openclaw = { id: 'r', connectionId: 'openclaw' } + const m4air = { id: 'r', connectionId: 'm4air' } + expect(createRepoRowExecutionHostLookup([openclaw, m4air]).byId('r')).toEqual({ + kind: 'ambiguous' + }) + + // Two rows agreeing on one host still resolve to the first in repo-list order. + const first: ExecutionHostOwnerRow = { id: 'r', connectionId: 'm4air' } + const second: ExecutionHostOwnerRow = { id: 'r', executionHostId: 'ssh:m4air' } + expect(createRepoRowExecutionHostLookup([first, second]).byId('r')).toEqual({ + kind: 'resolved', + owner: first + }) + }) + + it('keeps byHost hits, misses and repo-list order', () => { + const openclaw = { id: 'r', connectionId: 'openclaw' } + const m4air = { id: 'r', connectionId: 'm4air' } + const lookup = createRepoRowExecutionHostLookup([openclaw, m4air]) + expect(lookup.byHost('r', 'ssh:m4air')).toBe(m4air) + expect(lookup.byHost('r', 'ssh:openclaw')).toBe(openclaw) + expect(lookup.byHost('r', 'local')).toBeNull() + expect(lookup.byHost('other', 'local')).toBeNull() + }) +}) diff --git a/src/shared/worktree-execution-host-resolution.ts b/src/shared/worktree-execution-host-resolution.ts index 8abe5de72cc..00b66b7f11c 100644 --- a/src/shared/worktree-execution-host-resolution.ts +++ b/src/shared/worktree-execution-host-resolution.ts @@ -88,20 +88,37 @@ export function resolveWorktreeExecutionHost( } } -/** Array-backed lookup for callers holding the whole repo list (main's store). */ +const EMPTY_ROWS: readonly never[] = [] + +/** + * Array-backed lookup for callers holding the whole repo list (main's store). Grouped once at + * construction — a lookup is hit once per worktree key per target, so a per-call `filter` was an + * O(repos) rescan each time. Rows keep repo-list order, which `byId` depends on for `rows[0]`. + */ export function createRepoRowExecutionHostLookup( repos: readonly T[] ): ExecutionHostOwnerLookup { - const rowsFor = (repoId: string): T[] => repos.filter((repo) => repo.id === repoId) + const rowsById = new Map() + for (const repo of repos) { + const rows = rowsById.get(repo.id) + if (rows) { + rows.push(repo) + } else { + rowsById.set(repo.id, [repo]) + } + } + const rowsFor = (repoId: string): readonly T[] => rowsById.get(repoId) ?? EMPTY_ROWS return { byId: (repoId) => { const rows = rowsFor(repoId) - if (rows.length === 0) { + const owner = rows[0] + if (!owner) { return { kind: 'missing' } } - const hostIds = new Set(rows.map((repo) => getRepoExecutionHostId(repo))) - const owner = rows[0] - return hostIds.size > 1 || !owner ? { kind: 'ambiguous' } : { kind: 'resolved', owner } + const ownerHostId = getRepoExecutionHostId(owner) + return rows.some((repo) => getRepoExecutionHostId(repo) !== ownerHostId) + ? { kind: 'ambiguous' } + : { kind: 'resolved', owner } }, byHost: (repoId, hostId) => rowsFor(repoId).find((repo) => getRepoExecutionHostId(repo) === hostId) ?? null