From c7ae0f675473716281ab32a38bb22a934dcf3b07 Mon Sep 17 00:00:00 2001 From: ClaudiaFang Date: Tue, 14 Jul 2026 10:40:48 +0000 Subject: [PATCH] perf(push): parallelize blob creation within a batch commit User reported push-all was still slow on GitHub after the one- commit-per-run batching landed. Root cause: GitHubService/ GiteaService.pushBatch still created each file's blob with a sequential for loop -- one POST .../git/blobs awaited fully before the next started -- so wall-clock time was still O(N) round trips of pure latency even though only one commit came out the other end. - Add BaseGitService.mapWithConcurrency: an order-preserving, bounded-concurrency mapper (worker-pool over a shared cursor). - Add BLOB_CREATE_CONCURRENCY = 8 alongside MAX_BATCH_PUSH_SIZE. - GitHubService/GiteaService.pushBatch now create blobs via mapWithConcurrency instead of a for loop; the tree/commit/ref sequence after it is unchanged since each step genuinely depends on the previous one's result. - GitLab is unaffected -- its pushBatch already sends the whole batch in one Commits API request, no per-file blob step to parallelize. Added a regression-guard test: deferred blob-response promises that only resolve after every blob POST has been dispatched, so a reintroduced sequential loop deadlocks the test instead of silently passing. Evidence: npx eslint . -> 0 errors; npm run build -> clean; npx vitest run -> 340/340 passed. Co-Authored-By: Claude Sonnet 5 --- feature_list.json | 8 +++++ progress.md | 15 +++++++-- src/services/git-service-base.ts | 27 ++++++++++++++++ src/services/gitea-service.ts | 9 +++--- src/services/github-service.ts | 9 +++--- tests/services/github-service.test.ts | 44 +++++++++++++++++++++++++++ 6 files changed, 99 insertions(+), 13 deletions(-) diff --git a/feature_list.json b/feature_list.json index 99119ab..84391f5 100644 --- a/feature_list.json +++ b/feature_list.json @@ -97,6 +97,14 @@ "dependencies": ["feat-011"], "status": "done", "evidence": "npx eslint . -> 0 errors; npm run build -> clean; npx vitest run -> 339/339 passed; consolidated onto PR #51" + }, + { + "id": "feat-013", + "name": "perf(push): parallelize blob creation within a batch commit", + "description": "User reported push-all was still slow on GitHub after feat-011's one-commit-per-run batching landed. Root cause: GitHubService/GiteaService.pushBatch still created each file's blob with a sequential `for` loop (N sequential POST .../git/blobs round trips before the single tree/commit/ref sequence), so wall-clock time was still O(N) latency-bound. Added a shared BaseGitService.mapWithConcurrency helper (order-preserving, bounded concurrency) and a BLOB_CREATE_CONCURRENCY=8 cap in git-service-base.ts; both services' pushBatch now creates blobs concurrently instead of one at a time. GitLab unaffected (its Commits API already sends the whole batch in one request, no per-file blob step)", + "dependencies": ["feat-011"], + "status": "done", + "evidence": "npx eslint . -> 0 errors; npm run build -> clean; npx vitest run -> 340/340 passed; consolidated onto PR #51" } ], "_evidenceStyle": "Keep evidence to one line: commit hash + short pointer (e.g. 'Commit abc1234 - added X, tests pass'). Debugging narrative and design discussion belong in the commit message, not this file." diff --git a/progress.md b/progress.md index 2664b50..238c433 100644 --- a/progress.md +++ b/progress.md @@ -12,9 +12,9 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont ## Current State -**Last Updated:** 2026-07-14 10:30 +**Last Updated:** 2026-07-14 10:45 **Session ID:** current -**Active Feature:** feat-012 (perf: batch-commit remote-only file deletion) — done, lint/build/test all green. feat-011 (push-all batching) also done this session. Previous active item (issue #78 delete-remote-only-file fix) still awaiting user re-test after rebuild. +**Active Feature:** feat-013 (perf: parallelize blob creation in batch push) — done, lint/build/test all green. feat-011/feat-012 (push-all + delete batching) also done this session. Previous active item (issue #78 delete-remote-only-file fix) still awaiting user re-test after rebuild. ## Status @@ -54,6 +54,15 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont - Evidence: `npx eslint .` → 0 errors; `npm run build` → clean; `npx vitest run` → 339/339 passed. - Not yet committed as of this note — pending commit onto `claude/fix-directory-symlink-pull-260713`. +- [x] feat-013: perf(push): parallelize blob creation within a batch commit. User reported "batch push 還是很慢" (batch push is still slow) after feat-011/feat-012 landed; confirmed they were on GitHub with the latest build. Root cause found by re-reading `GitHubService.pushBatch`/`GiteaService.pushBatch`: the blob-creation step was still a sequential `for` loop — one `POST .../git/blobs` awaited fully before the next started — so wall-clock time for the batch was still O(N) network round trips of pure latency, even though only one commit was produced at the end. This was a known, called-out tradeoff from feat-011's plan ("sequential is fine... could be Promise.all'd for extra speed, but... note as a possible future optimization"); the user's report made it worth doing now. + - Added `BaseGitService.mapWithConcurrency(items, concurrency, fn)` (`git-service-base.ts`): order-preserving, bounded-concurrency mapper (worker-pool pattern over a shared cursor, `Promise.all` over `Math.min(concurrency, items.length)` workers). + - Added `BLOB_CREATE_CONCURRENCY = 8` constant alongside `MAX_BATCH_PUSH_SIZE`. + - `GitHubService.pushBatch`/`GiteaService.pushBatch` now build blobs via `mapWithConcurrency` instead of a `for` loop; the tree/commit/ref sequence after it is unchanged (genuinely sequential — each step depends on the previous one's result). + - GitLab unaffected — its `pushBatch` already sends the whole batch in one Commits API request, no per-file blob step to parallelize. + - Added a regression-guard test (`github-service.test.ts`): deferred blob-response promises that only resolve after every blob POST has been dispatched — a reintroduced sequential loop would deadlock on this test (call 2 never fires until call 1's promise resolves, which it doesn't), so it fails loudly if someone reverts to sequential. + - Evidence: `npx eslint .` → 0 errors; `npm run build` → clean; `npx vitest run` → 340/340 passed. + - Not yet committed as of this note — pending commit onto `claude/fix-directory-symlink-pull-260713`. + ### What's In Progress - Nothing else actively in progress. @@ -61,7 +70,7 @@ Completed work is archived in [archive/](./archive/), one file per calendar mont ### What's Next 1. Get user confirmation that delete now works (or a new detailed error message) after they rebuild/reload with the latest push (issue #78). -2. Manually verify feat-011/feat-012 in Obsidian if possible: push-all and batch-delete on a mixed vault should each produce exactly one new commit. +2. Manually verify feat-011/feat-012/feat-013 in Obsidian if possible: push-all and batch-delete on a mixed vault should each produce exactly one new commit, and push-all should now feel noticeably faster on GitHub/Gitea (blobs created concurrently, up to 8 in flight). 3. Issue #37 (Bitbucket provider support, feat-010) — large, was deferred until #38 (i18n) landed. Now unblocked. Its `GitServiceInterface` implementation should simply omit `pushBatch`/`deleteBatch` and use the existing per-file fallbacks. 4. Re-sync against `gh issue list --repo firstsun-dev/git-files-sync --state open`. Remaining genuinely-unstarted issues as of this session: #47 (regex ignore lists), #45 (SonarQube findings), #37 (Bitbucket), #28 (non-engineering: community visibility). 5. PR #51 is large (8+ issues' worth of changes now). If the user wants to review/merge it before more work piles on, flag this rather than continuing to add commits indefinitely. diff --git a/src/services/git-service-base.ts b/src/services/git-service-base.ts index 86b5068..b33e57c 100644 --- a/src/services/git-service-base.ts +++ b/src/services/git-service-base.ts @@ -59,6 +59,14 @@ export interface ConnectionTestResult { * bodies / provider payload limits when a vault has thousands of files. */ export const MAX_BATCH_PUSH_SIZE = 200; +/** How many blob-creation requests to have in flight at once when building a + * batch commit. Creating each file's blob is an independent request with no + * ordering dependency, so running them one-at-a-time (as opposed to the final + * tree/commit/ref sequence, which genuinely is sequential) just adds N + * round trips of pure latency. A moderate cap keeps this fast without + * bursting past a provider's abuse-detection/secondary rate limits. */ +export const BLOB_CREATE_CONCURRENCY = 8; + export abstract class BaseGitService { protected token: string = ''; protected rootPath: string = ''; @@ -320,6 +328,25 @@ export abstract class BaseGitService { return newCommitSha; } + /** + * Runs `fn` over `items` with at most `concurrency` calls in flight at + * once, preserving result order. Used to parallelize independent + * per-file requests (e.g. blob creation) that would otherwise pay N + * round trips of latency running one at a time. + */ + protected async mapWithConcurrency(items: T[], concurrency: number, fn: (item: T, index: number) => Promise): Promise { + const results = new Array(items.length); + let nextIndex = 0; + const worker = async (): Promise => { + while (nextIndex < items.length) { + const i = nextIndex++; + results[i] = await fn(items[i] as T, i); + } + }; + await Promise.all(Array.from({ length: Math.min(concurrency, items.length) }, worker)); + return results; + } + async getRepoGitignores(branch: string): Promise { try { const allFiles = await this.listFiles(branch, false); // Fetch ALL files to find gitignores diff --git a/src/services/gitea-service.ts b/src/services/gitea-service.ts index 632778c..e794365 100644 --- a/src/services/gitea-service.ts +++ b/src/services/gitea-service.ts @@ -1,5 +1,5 @@ import { GitServiceInterface, GitTreeEntry, BatchPushItem, BatchPushResult } from './git-service-interface'; -import { BaseGitService, ConnectionTestResult, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE } from './git-service-base'; +import { BaseGitService, ConnectionTestResult, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE, BLOB_CREATE_CONCURRENCY } from './git-service-base'; import { logger } from '../utils/logger'; export class GiteaService extends BaseGitService implements GitServiceInterface { @@ -80,14 +80,13 @@ export class GiteaService extends BaseGitService implements GitServiceInterface const base = this.getGitDataApiBase(); const { latestCommitSha, baseTreeSha } = await this.resolveBaseTree(branch); - const blobShas: string[] = []; - for (const item of items) { + const blobShas = await this.mapWithConcurrency(items, BLOB_CREATE_CONCURRENCY, async item => { const blobResp = await this.safeRequest(`${base}/git/blobs`, 'POST', { content: this.encodeContent(item.content), encoding: 'base64', }); - blobShas.push(this.parseJson<{ sha: string }>(blobResp).sha); - } + return this.parseJson<{ sha: string }>(blobResp).sha; + }); const treeItems = items.map((item, i) => ({ path: this.getFullPath(item.path), diff --git a/src/services/github-service.ts b/src/services/github-service.ts index e56ebd6..53d4ae5 100644 --- a/src/services/github-service.ts +++ b/src/services/github-service.ts @@ -1,5 +1,5 @@ import { GitServiceInterface, GitTreeEntry, BatchPushItem, BatchPushResult } from './git-service-interface'; -import { BaseGitService, ConnectionTestResult, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE } from './git-service-base'; +import { BaseGitService, ConnectionTestResult, GitFile, GitHubContentResponse, GitHubTreeResponse, GIT_SYMLINK_MODE, BLOB_CREATE_CONCURRENCY } from './git-service-base'; import { logger } from '../utils/logger'; export class GitHubService extends BaseGitService implements GitServiceInterface { @@ -92,14 +92,13 @@ export class GitHubService extends BaseGitService implements GitServiceInterface const base = this.getGitDataApiBase(); const { latestCommitSha, baseTreeSha } = await this.resolveGitHubStyleBaseTree(branch); - const blobShas: string[] = []; - for (const item of items) { + const blobShas = await this.mapWithConcurrency(items, BLOB_CREATE_CONCURRENCY, async item => { const blobResp = await this.safeRequest(`${base}/git/blobs`, 'POST', { content: this.encodeContent(item.content), encoding: 'base64', }); - blobShas.push(this.parseJson<{ sha: string }>(blobResp).sha); - } + return this.parseJson<{ sha: string }>(blobResp).sha; + }); const treeItems = items.map((item, i) => ({ path: this.getFullPath(item.path), diff --git a/tests/services/github-service.test.ts b/tests/services/github-service.test.ts index d4f059b..2757d07 100644 --- a/tests/services/github-service.test.ts +++ b/tests/services/github-service.test.ts @@ -144,6 +144,50 @@ describe('GitHubService', () => { expect(calls[6]?.method).toBe('PATCH'); expect(JSON.parse(calls[6]?.body as string)).toEqual({ sha: 'commit2' }); }); + + it('creates blobs concurrently, not one at a time (regression guard against a sequential loop)', async () => { + // Deferred blob responses: none of them resolve until every blob + // POST has already been dispatched. A sequential `for` loop would + // deadlock here since call N+1 never fires until call N resolves. + const blobDeferreds = Array.from({ length: 4 }, () => { + let resolve!: (v: RequestUrlResponse) => void; + const promise = new Promise(r => { resolve = r; }); + return { promise, resolve }; + }); + let blobCallCount = 0; + + vi.mocked(requestUrl).mockImplementation(((param: string | RequestUrlParam) => { + const url = (param as RequestUrlParam).url; + if (url.endsWith('/git/ref/heads/main')) { + return Promise.resolve({ status: 200, json: { object: { sha: 'commit1' } } } as unknown as RequestUrlResponse); + } + if (url.includes('/git/commits/commit1')) { + return Promise.resolve({ status: 200, json: { tree: { sha: 'tree1' } } } as unknown as RequestUrlResponse); + } + if (url.endsWith('/git/blobs')) { + return blobDeferreds[blobCallCount++]!.promise; + } + if (url.endsWith('/git/trees')) { + return Promise.resolve({ status: 201, json: { sha: 'tree2' } } as unknown as RequestUrlResponse); + } + if (url.endsWith('/git/commits')) { + return Promise.resolve({ status: 201, json: { sha: 'commit2' } } as unknown as RequestUrlResponse); + } + return Promise.resolve({ status: 200, json: {} } as unknown as RequestUrlResponse); + }) as unknown as typeof requestUrl); + + const items = Array.from({ length: 4 }, (_, i) => ({ path: `f${i}.md`, content: `content${i}` })); + const resultPromise = service.pushBatch(items, 'main', 'push'); + + // Wait for every blob POST to have been dispatched — a sequential + // loop would never get past the first one, since its promise never resolves. + await vi.waitFor(() => expect(blobCallCount).toBe(4)); + + blobDeferreds.forEach((d, i) => d.resolve({ status: 201, json: { sha: `blob-${i}` } } as unknown as RequestUrlResponse)); + const result = await resultPromise; + + expect(result).toEqual(items.map((item, i) => ({ path: item.path, sha: `blob-${i}` }))); + }); }); describe('pushFile', () => {