perf(push): eliminate redundant GitHub requests

Author: ClaudiaFang

docs/push-strategy-benchmark.md

@@ -0,0 +1,25 @@
+# GitHub push strategy benchmark (#61)
+
+## Procedure
+
+Use a disposable repository and branch. For each file count (1, 5, 10, 25, 50, 100, 200), generate equal-size unique files, run the GraphQL path and then the developer-only `pushBatchViaGitDataApiForBenchmark` path on separate branches, and record the opt-in `PushTimingRecord` plus the REST wall time. Do not run this against a user vault or production branch.
+
+The GraphQL record is enabled only by registering `setPushTimingHandler`; it contains no file paths, contents, token, repository identity, or network transmission. `providerProcessingMs` is intentionally `null`, because Obsidian's `requestUrl` API does not expose server-only timing.
+
+## Deterministic request-count result
+
+| Files | GraphQL requests | Git Data API requests | Git Data API request waves (8 concurrent blobs) |
+| ---: | ---: | ---: | ---: |
+| 1 | 2 | 6 | 6 |
+| 5 | 2 | 10 | 6 |
+| 10 | 2 | 15 | 7 |
+| 25 | 2 | 30 | 9 |
+| 50 | 2 | 55 | 12 |
+| 100 | 2 | 105 | 18 |
+| 200 | 2 | 205 | 30 |
+
+GraphQL performs a branch-head read and one mutation; the caller marks the committed paths synced without a tree readback. Git Data API performs a ref read, commit read, one blob upload per file, tree creation, commit creation, and ref update. The table is covered by service tests for both implementations.
+
+## Decision
+
+Keep GraphQL as the production strategy for every supported batch size (1–200). There is no request-count crossover: Git Data API starts with four additional round trips and adds one request per file. The REST path remains developer-only so provider benchmarks can challenge this conclusion without adding a user-facing strategy switch.

src/logic/sync-manager.ts

@@ -190,13 +190,8 @@ export class SyncManager {
             );
 
             // Update metadata
-            let newSha = result.sha;
-            if (!newSha) {
-                const newRemote = await this.gitService.getFile(repoPath, this.settings.branch);
-                newSha = newRemote.sha;
-            }
-            
-            if (newSha) await this.updateMetadata(file.path, newSha);
+            const newSha = result.sha ?? await gitBlobSha(content);
+            await this.updateMetadata(file.path, newSha);
 
             // Remove old metadata
             delete this.settings.syncMetadata[oldPath];
@@ -220,13 +215,8 @@ export class SyncManager {
         );
 
         // Update metadata
-        let newSha = result.sha;
-        if (!newSha) {
-            const newRemote = await this.gitService.getFile(repoPath, this.settings.branch);
-            newSha = newRemote.sha;
-        }
-
-        if (newSha) await this.updateMetadata(file.path, newSha);
+        const newSha = result.sha ?? await gitBlobSha(content);
+        await this.updateMetadata(file.path, newSha);
 
         if (!silent) new Notice(`Pushed ${file.name} to ${this.serviceName}`);
         return newSha;

src/services/git-service-interface.ts

@@ -46,6 +46,8 @@ export interface GitServiceInterface {
     updateConfig(...args: unknown[]): void;
     getFile(path: string, branch: string): Promise<GitFile>;
     pushFile(path: string, content: string | ArrayBuffer, branch: string, commitMessage: string, existingSha?: string): Promise<{ path: string, sha?: string }>;
+    /** Returns the branch's current commit SHA when the provider can expose it cheaply. */
+    getBranchHead?(branch: string): Promise<string>;
     /** Checks the repository is reachable and the given branch exists. */
     testConnection(branch: string): Promise<ConnectionTestResult>;
     listFiles(branch: string, useFilter?: boolean): Promise<string[]>;

src/services/github-service.ts

@@ -1,6 +1,7 @@
 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';
+import { PushTimingCollector, PushTimingHandler, PushTimingRecord } from './push-timing';
 
 /**
  * Commits any mix of file additions/deletions in one request. Used instead of
@@ -20,6 +21,12 @@ const CREATE_COMMIT_MUTATION = `
 export class GitHubService extends BaseGitService implements GitServiceInterface {
     private owner: string = '';
     private repo: string = '';
+    private pushTimingHandler?: PushTimingHandler;
+
+    /** Enables local diagnostic records; the plugin itself never enables this. */
+    setPushTimingHandler(handler?: PushTimingHandler): void {
+        this.pushTimingHandler = handler;
+    }
 
     updateConfig(token: string, owner: string, repo: string, rootPath: string = '') {
         this.token = token;
@@ -42,6 +49,10 @@ export class GitHubService extends BaseGitService implements GitServiceInterface
         return `https://api.github.com/repos/${this.owner}/${this.repo}`;
     }
 
+    async getBranchHead(branch: string): Promise<string> {
+        return this.getLatestCommitSha(branch);
+    }
+
     async getFile(path: string, branch: string): Promise<GitFile> {
         try {
             const url = `${this.getApiUrl(path)}?ref=${branch}`;
@@ -64,21 +75,9 @@ export class GitHubService extends BaseGitService implements GitServiceInterface
         }
     }
 
-    async pushFile(path: string, content: string | ArrayBuffer, branch: string, message: string, sha?: string): Promise<{ path: string, sha?: string }> {
-        const url = this.getApiUrl(path);
-        const body: { message: string; content: string; branch: string; sha?: string } = {
-            message,
-            content: this.encodeContent(content),
-            branch,
-        };
-        // GitHub's Contents API rejects a blank sha with HTTP 422. Only include
-        // it when updating an existing file; a 404 lookup yields sha === '' for
-        // new files, which must be created without a sha.
-        if (sha) body.sha = sha;
-
-        const response = await this.safeRequest(url, 'PUT', body);
-        const data = this.parseJson<{ content: { path: string, sha: string } }>(response);
-        return { path: data.content.path, sha: data.content.sha };
+    async pushFile(path: string, content: string | ArrayBuffer, branch: string, message: string, _existingSha?: string): Promise<{ path: string, sha?: string }> {
+        const [result] = await this.pushBatch([{ path, content }], branch, message);
+        return result ?? { path };
     }
 
     async pushSymlink(path: string, target: string, branch: string, message: string): Promise<{ path: string, sha?: string }> {
@@ -108,9 +107,11 @@ export class GitHubService extends BaseGitService implements GitServiceInterface
      * rather than an HTTP error status, so this checks for that on top of
      * safeRequest's status-code check.
      */
-    private async githubGraphQL<T>(query: string, variables: Record<string, unknown>): Promise<T> {
-        const response = await this.safeRequest('https://api.github.com/graphql', 'POST', { query, variables });
-        const body = this.parseJson<{ data?: T; errors?: Array<{ message: string }> }>(response);
+    private async githubGraphQL<T>(query: string, variables: Record<string, unknown>, timing?: PushTimingCollector): Promise<T> {
+        const request = () => this.safeRequest('https://api.github.com/graphql', 'POST', { query, variables });
+        const response = timing ? await timing.measureRequest(request) : await request();
+        const parse = () => this.parseJson<{ data?: T; errors?: Array<{ message: string }> }>(response);
+        const body = timing ? timing.measureParsing(parse) : parse();
         if (body.errors && body.errors.length > 0) {
             throw new Error(`GitHub GraphQL error: ${body.errors.map(e => e.message).join('; ')}`);
         }
@@ -130,10 +131,11 @@ export class GitHubService extends BaseGitService implements GitServiceInterface
      * an obvious staleness error. A short retry with a freshly re-read HEAD
      * self-heals once GitHub's read catches up.
      */
-    private async commitOnBranch(branch: string, message: string, fileChanges: Record<string, unknown>): Promise<string> {
+    private async commitOnBranch(branch: string, message: string, fileChanges: Record<string, unknown>, timing?: PushTimingCollector): Promise<string> {
         const maxAttempts = 3;
         for (let attempt = 1; attempt <= maxAttempts; attempt++) {
-            const expectedHeadOid = await this.getLatestCommitSha(branch);
+            const getHead = () => this.getLatestCommitSha(branch);
+            const expectedHeadOid = timing ? await timing.measureRequest(getHead) : await getHead();
             try {
                 const data = await this.githubGraphQL<{ createCommitOnBranch: { commit: { oid: string } } }>(CREATE_COMMIT_MUTATION, {
                     input: {
@@ -142,7 +144,7 @@ export class GitHubService extends BaseGitService implements GitServiceInterface
                         expectedHeadOid,
                         fileChanges,
                     },
-                });
+                }, timing);
                 return data.createCommitOnBranch.commit.oid;
             } catch (e) {
                 const errorMessage = e instanceof Error ? e.message : String(e);
@@ -157,31 +159,71 @@ export class GitHubService extends BaseGitService implements GitServiceInterface
 
     async pushBatch(items: BatchPushItem[], branch: string, message: string): Promise<BatchPushResult[]> {
         if (items.length === 0) return [];
+        const timing = this.pushTimingHandler ? new PushTimingCollector() : undefined;
+        const preparationStartedAt = performance.now();
+        const preparedItems = items.map(item => ({ item, path: this.getFullPath(item.path) }));
+        const rawBytes = items.reduce((total, item) => total + this.getByteLength(item.content), 0);
+        const changePreparationMs = performance.now() - preparationStartedAt;
+        const encodingStartedAt = performance.now();
+        const additions = preparedItems.map(({ item, path }) => ({ path, contents: this.encodeContent(item.content) }));
+        const encodedBytes = additions.reduce((total, addition) => total + this.getByteLength(addition.contents), 0);
+        const encodingMs = performance.now() - encodingStartedAt;
+        let failure: unknown;
 
-        await this.commitOnBranch(branch, message, {
-            additions: items.map(item => ({
-                path: this.getFullPath(item.path),
-                contents: this.encodeContent(item.content),
-            })),
-        });
+        try {
+            await this.commitOnBranch(branch, message, { additions }, timing);
+            // The caller already marks committed paths as synced. Avoiding a
+            // full recursive tree read saves a request and sidesteps GitHub's
+            // briefly stale tree reads after a successful mutation.
+            return items.map(item => ({ path: item.path }));
+        } catch (error) {
+            failure = error;
+            throw error;
+        } finally {
+            this.emitPushTiming(timing, 'github-graphql', items.length, rawBytes, encodedBytes, changePreparationMs, encodingMs, failure);
+        }
+    }
+
+    /**
+     * Developer-only Git Data API control path for benchmark #61. Production
+     * pushes continue to use GraphQL because this path requires one blob POST
+     * per file. It is intentionally not part of GitServiceInterface.
+     */
+    async pushBatchViaGitDataApiForBenchmark(items: BatchPushItem[], branch: string, message: string): Promise<BatchPushResult[]> {
+        if (items.length === 0) return [];
 
-        // createCommitOnBranch only returns the new commit's oid, not each
-        // file's blob sha, so read them back with a follow-up tree fetch
-        // (mirrors GitLab's pushBatch, which has the same limitation). That
-        // fetch is exposed to the same eventual-consistency lag the retry
-        // above works around, so a fresh tree can still be briefly missing an
-        // entry that was just committed; retry it too rather than silently
-        // returning an undefined sha for that file.
+        const base = this.getGitDataApiBase();
+        const { latestCommitSha, baseTreeSha } = await this.resolveGitHubStyleBaseTree(branch);
         const fullPaths = items.map(item => this.getFullPath(item.path));
-        for (let attempt = 1; attempt <= 3; attempt++) {
-            const freshTree = await this.listFilesDetailed(branch, false);
-            const shaByPath = new Map(freshTree.map(e => [e.path, e.sha]));
-            const results = items.map((item, i) => ({ path: item.path, sha: shaByPath.get(fullPaths[i] as string) }));
-            if (results.every(r => r.sha) || attempt === 3) return results;
-            await new Promise(resolve => window.setTimeout(resolve, 500 * attempt));
-        }
-        // Unreachable: the loop always returns on its last iteration.
-        throw new Error('pushBatch: exhausted retries reading back blob shas');
+        const blobShas = await this.mapWithConcurrency(items, BLOB_CREATE_CONCURRENCY, async item => {
+            const response = await this.safeRequest(`${base}/git/blobs`, 'POST', {
+                content: this.encodeContent(item.content),
+                encoding: 'base64',
+            });
+            return this.parseJson<{ sha: string }>(response).sha;
+        });
+
+        await this.commitGitHubStyleTree(
+            base, branch, baseTreeSha, latestCommitSha,
+            fullPaths.map((path, index) => ({ path, mode: '100644', type: 'blob' as const, sha: blobShas[index] as string })),
+            message
+        );
+        return items.map((item, index) => ({ path: item.path, sha: blobShas[index] }));
+    }
+
+    private getByteLength(content: string | ArrayBuffer): number {
+        return typeof content === 'string' ? new TextEncoder().encode(content).byteLength : content.byteLength;
+    }
+
+    private getErrorMessage(error: unknown): string | undefined {
+        if (error === undefined) return undefined;
+        return error instanceof Error ? error.message : 'Non-Error push failure';
+    }
+
+    private emitPushTiming(timing: PushTimingCollector | undefined, strategy: PushTimingRecord['strategy'], fileCount: number, rawBytes: number, encodedBytes: number, changePreparationMs: number, encodingMs: number, error?: unknown): void {
+        if (!timing || !this.pushTimingHandler) return;
+        const failure = this.getErrorMessage(error);
+        this.pushTimingHandler(timing.createRecord(strategy, fileCount, rawBytes, encodedBytes, changePreparationMs, encodingMs, failure));
     }
 
     async listFilesDetailed(branch: string, useFilter = true): Promise<GitTreeEntry[]> {

src/services/push-timing.ts

@@ -0,0 +1,61 @@
+/**
+ * Developer-facing, telemetry-free measurements for one batched push.
+ * The plugin never stores or transmits these records; callers opt in by
+ * registering a handler on GitHubService.
+ */
+export interface PushTimingRecord {
+    strategy: 'github-graphql' | 'github-git-data';
+    fileCount: number;
+    rawBytes: number;
+    encodedBytes: number;
+    changePreparationMs: number;
+    encodingMs: number;
+    requestUploadMs: number;
+    responseParsingMs: number;
+    /** requestUrl does not expose server-only timing, so it is intentionally null. */
+    providerProcessingMs: null;
+    requestCount: number;
+    totalMs: number;
+    failure?: string;
+}
+
+export type PushTimingHandler = (record: PushTimingRecord) => void;
+
+export class PushTimingCollector {
+    private readonly startedAt = performance.now();
+    private requestUploadMs = 0;
+    private responseParsingMs = 0;
+    private requestCount = 0;
+
+    async measureRequest<T>(operation: () => Promise<T>): Promise<T> {
+        const startedAt = performance.now();
+        try {
+            return await operation();
+        } finally {
+            this.requestUploadMs += performance.now() - startedAt;
+            this.requestCount++;
+        }
+    }
+
+    measureParsing<T>(operation: () => T): T {
+        const startedAt = performance.now();
+        try {
+            return operation();
+        } finally {
+            this.responseParsingMs += performance.now() - startedAt;
+        }
+    }
+
+    createRecord(
+        strategy: PushTimingRecord['strategy'], fileCount: number, rawBytes: number,
+        encodedBytes: number, changePreparationMs: number, encodingMs: number, failure?: string
+    ): PushTimingRecord {
+        return {
+            strategy, fileCount, rawBytes, encodedBytes, changePreparationMs, encodingMs,
+            requestUploadMs: this.requestUploadMs, responseParsingMs: this.responseParsingMs,
+            providerProcessingMs: null, requestCount: this.requestCount,
+            totalMs: performance.now() - this.startedAt,
+            ...(failure ? { failure } : {}),
+        };
+    }
+}