From 71f3700aecbf2694b54dbe6dbb0bc50fb6d480a3 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 03:16:58 -0700 Subject: [PATCH] perf(remote): read the repo catalog once per publish, not once per worktree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `remoteWorkspace:setForConnectedTargets` costs 13 ms of main-thread time per call at 0.48 calls/sec — 0.63% of wall on a real session, the second most expensive IPC handler in the main process. Almost all of it is one line. `exportRemoteWorkspaceSession` asks `isTargetWorktree(worktreeId)` once per worktree in the session, and that callback called `targetForWorktree(store, ...)`, which called `store.getRepos()` — and `getRepos()` maps `hydrateRepo` over every repo row. So publishing to one SSH target re-hydrated the whole repo catalog once per worktree, then threw a fresh `createRepoRowExecutionHostLookup` (which itself `filter`s the catalog per lookup) away each time. The lookup is now built once per handler invocation and shared across targets: the rows cannot change inside one synchronous projection, and they are the same for every target. On the session that surfaced this — 413 worktrees, 13 repos, 1 connected target — that is 413 catalog hydrations (5369 `hydrateRepo` calls) per publish reduced to 1 (13 calls). The repo normaliser reached through `hydrateRepo` was the #2 self-time function in a 30 s main-process CPU profile at 0.25%. No user-facing trade-off: identical ownership resolution, identical exported session, identical stale-revision handling. --- src/main/ipc/remote-workspace.test.ts | 29 ++++++++++++++++++++++++++- src/main/ipc/remote-workspace.ts | 25 +++++++++++++++-------- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/main/ipc/remote-workspace.test.ts b/src/main/ipc/remote-workspace.test.ts index 56eb4804a82..9a25ada9ec7 100644 --- a/src/main/ipc/remote-workspace.test.ts +++ b/src/main/ipc/remote-workspace.test.ts @@ -154,9 +154,10 @@ describe('remoteWorkspace:setForConnectedTargets', () => { const getRepoMock = vi.fn() const getWorkspaceSessionMock = vi.fn() // Ownership resolution reads the catalog, not one id-keyed row, so the fake has to project one. + const getReposMock = vi.fn(() => [getRepoMock('repo-target-1')].filter(Boolean)) const store = { getRepo: getRepoMock, - getRepos: () => [getRepoMock('repo-target-1')].filter(Boolean), + getRepos: getReposMock, getWorkspaceSession: getWorkspaceSessionMock } as unknown as Store @@ -176,6 +177,7 @@ describe('remoteWorkspace:setForConnectedTargets', () => { getTarget: (targetId: string) => targets.find((target) => target.id === targetId) }) getRepoMock.mockReset() + getReposMock.mockClear() getWorkspaceSessionMock.mockReset() getWorkspaceSessionMock.mockReturnValue(baseSession) getRepoMock.mockImplementation((repoId: string) => @@ -251,6 +253,31 @@ describe('remoteWorkspace:setForConnectedTargets', () => { return observed as RemoteWorkspaceObservedSnapshot } + it('reads the repo catalog once per publish, not once per worktree', async () => { + // `store.getRepos()` re-hydrates every repo row. The export asks "is this worktree mine?" once + // per worktree, so reading the catalog inside that callback multiplied hydration by the + // worktree count — 413 on the session that surfaced this. + const worktrees = Object.fromEntries( + Array.from({ length: 12 }, (_, index) => [`repo-target-1::/remote/repo-${index}`, []]) + ) + getWorkspaceSessionMock.mockReturnValue({ + ...baseSession, + tabsByWorktree: worktrees + } as WorkspaceSessionState) + const observed = await observeTarget('target-1') + getReposMock.mockClear() + + await callSetForConnectedTargets({ + hydratedTargetIds: ['target-1'], + expectedRevisionsByTargetId: { 'target-1': observed.snapshot?.revision ?? 7 }, + expectedHostObservationTokensByTargetId: { + 'target-1': observed.hostObservationToken + } + }) + + expect(getReposMock).toHaveBeenCalledTimes(1) + }) + it('does not write without an explicit non-empty hydrated target set', async () => { await expect(callSetForConnectedTargets({ session: baseSession })).resolves.toEqual([]) await expect( diff --git a/src/main/ipc/remote-workspace.ts b/src/main/ipc/remote-workspace.ts index f9479f15ce9..a530e55105c 100644 --- a/src/main/ipc/remote-workspace.ts +++ b/src/main/ipc/remote-workspace.ts @@ -1,5 +1,6 @@ import { ipcMain, type BrowserWindow } from 'electron' import type { Store } from '../persistence' +import type { Repo } from '../../shared/repo-types' import { getActiveMultiplexer, getSshConnectionStore } from './ssh' import { exportRemoteWorkspaceSession } from '../../shared/remote-workspace-session-projection' import { @@ -107,7 +108,7 @@ function getExpectedHostObservationTokens( } function targetForWorktree( - store: Store, + repoLookup: ReturnType>, worktreeId: string, executionHostId?: string ): string | null { @@ -115,21 +116,27 @@ function targetForWorktree( // `getRepo(id)?.connectionId`, which is host-blind — the same repo id can name rows on several // hosts, so a session could be published to a machine that never owned the worktree (#11163). // Unresolvable ownership exports to nobody rather than guessing. - const resolution = resolveWorktreeExecutionHost( - createRepoRowExecutionHostLookup(store.getRepos()), - { repoId: getRepoIdFromWorktreeId(worktreeId), hostId: executionHostId ?? null } - ) + const resolution = resolveWorktreeExecutionHost(repoLookup, { + repoId: getRepoIdFromWorktreeId(worktreeId), + hostId: executionHostId ?? null + }) return resolution.kind === 'resolved' ? resolution.connectionId : null } +/** + * Why the lookup is built by the caller: `isTargetWorktree` runs once per worktree in the session, + * and `store.getRepos()` re-hydrates every repo row on each call. Reading the repo list once per + * export — the rows cannot change inside one synchronous projection — keeps that hydration off the + * per-worktree path. + */ function exportSessionForTarget( - store: Store, + repoLookup: ReturnType>, targetId: string, session: WorkspaceSessionState ): RemoteWorkspaceSession { return exportRemoteWorkspaceSession(session, { isTargetWorktree: (worktreeId, executionHostId) => - targetForWorktree(store, worktreeId, executionHostId) === targetId + targetForWorktree(repoLookup, worktreeId, executionHostId) === targetId }) } @@ -246,11 +253,13 @@ export function registerRemoteWorkspaceHandlers( ) ?? [] const workspaceSession = args.session ?? store.getWorkspaceSession() + // One repo read for every target: the rows are the same for all of them. + const repoLookup = createRepoRowExecutionHostLookup(store.getRepos()) const results = await Promise.all( targets.map(async (target) => { // Why: each target has its own revision stream. Keep same-target // writes queued, but do not let one slow relay block others. - const session = exportSessionForTarget(store, target.id, workspaceSession) + const session = exportSessionForTarget(repoLookup, target.id, workspaceSession) const result = await queueRemoteWorkspacePatch(target.id, async () => { const current = getCachedRemoteWorkspaceSnapshot(target.id) ?? (await getRemoteSnapshot(target))