mirror of
https://github.com/firstsun-dev/git-files-sync.git
synced 2026-07-22 06:54:27 +00:00
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<T,R>: 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 <noreply@anthropic.com>
This commit is contained in:
parent
83499c92e8
commit
c7ae0f6754
6 changed files with 99 additions and 13 deletions
|
|
@ -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."
|
||||
|
|
|
|||
15
progress.md
15
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<T,R>(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.
|
||||
|
|
|
|||
|
|
@ -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<T, R>(items: T[], concurrency: number, fn: (item: T, index: number) => Promise<R>): Promise<R[]> {
|
||||
const results = new Array<R>(items.length);
|
||||
let nextIndex = 0;
|
||||
const worker = async (): Promise<void> => {
|
||||
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<string[]> {
|
||||
try {
|
||||
const allFiles = await this.listFiles(branch, false); // Fetch ALL files to find gitignores
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
|
|
|
|||
|
|
@ -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<RequestUrlResponse>(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', () => {
|
||||
|
|
|
|||
Loading…
Reference in a new issue