From d6924853c32c7384fb241603657f5a336d61563a Mon Sep 17 00:00:00 2001 From: Adib Hanna Date: Mon, 26 Oct 2026 15:34:54 -0500 Subject: [PATCH] perf(mobile): Cloud pulls make 10 bridge calls per file instead of 14 A long first Cloud sync spends nearly all its time in Capacitor bridge round trips, about 14 per pulled file on the iOS Simulator. Four of them guarded nothing: the backup delete after a write that made no backup, the temporary check after the rename that already moved it, the recovery listing before every write, and the mkdir before every rename into a folder this sync already made. The pre-publish stat, both verifies, the original's verify and every rollback stay. Recovery files are listed again after a failed write, a backup that would not delete, or any staged publish, which makes a staged publish's leftover rollback copy stop the next write in the same run. Same change as ZenNotes/zennotesandroid#133, less the getUri trim, which this shell never needed. First sync medians on the simulator: 600 notes 4.34 -> 3.49 s, 2,500 notes 17.24 -> 14.66 s, vault files identical. The write-up is docs/cloud-pull-bridge-calls-2026-10-26.md; six new tests pin the call counts and the recovery rechecks. --- docs/cloud-pull-bridge-calls-2026-10-26.md | 164 ++++++++++++++++++ src/bridge/cloud-sync-repository.test.ts | 128 ++++++++++++++ src/bridge/cloud-sync-repository.ts | 27 ++- .../mobile-cloud-sync.integration.test.ts | 18 ++ src/bridge/mobile-cloud-sync.ts | 17 +- 5 files changed, 347 insertions(+), 7 deletions(-) create mode 100644 docs/cloud-pull-bridge-calls-2026-10-26.md diff --git a/docs/cloud-pull-bridge-calls-2026-10-26.md b/docs/cloud-pull-bridge-calls-2026-10-26.md new file mode 100644 index 0000000..7cb2917 --- /dev/null +++ b/docs/cloud-pull-bridge-calls-2026-10-26.md @@ -0,0 +1,164 @@ +# Cloud pull: fewer bridge calls per file, 26 October 2026 + +Branch `perf/cloud-pull-bridge-calls`, cut from `origin/main` at 7caa474 +(1.32.0 and two later fixes). Changes: `src/bridge/cloud-sync-repository.ts`, +`src/bridge/mobile-cloud-sync.ts`, and tests. No native code, no Capacitor +config and no app-core change. This is the iPhone half of +ZenNotes/zennotesandroid#133. + +## Why + +A Discord report (yeminn) had phones sitting in "Syncing..." through a long +first Cloud sync. A pull costs the same per file whatever the vault size, and +that cost is almost all Capacitor bridge round trips, made one after another: +on the iOS Simulator about 14 per pulled file, and the time spent waiting on +them adds up to nearly the whole sync. So the lever for big first syncs is +calls per file. + +## What changed + +Four calls per pulled file were redundant. Each is gone only where it could +not have guarded a file. + +| Call | Where | Why it cannot lose a file | +|---|---|---| +| `deleteFile(backup)` on every write | `NativeCloudSyncContent.write` | A backup exists only when an original was moved aside (`before`). A new file never makes one, so the delete always failed into its `.catch`. Replacing a file still deletes its backup. | +| `stat(temporary)` after publishing | same | A rename that resolved moved the temporary onto the path. Checked on the simulator: after a resolved `Filesystem.rename` the source is gone and the destination is there. Every path that did not publish still checks and removes the temporary. | +| `readdir(.zennotes/sync)` on every write | `assertReady` | Recovery files are now relisted only when one may have appeared: after a write that failed, after a backup that would not delete, and after any staged publish (which keeps its own backups). The first write, every scan and every staged download still list first. A completed write that removed its backup leaves nothing to find. | +| `mkdir(parent)` before every rename | `hostVault` in `mobile-cloud-sync.ts` | Each sync operation remembers the folders it has made. A rename into a folder that went missing fails before it changes anything (checked on the simulator: `OS-PLUG-FILE-0013`, the source stays, no folder is made), and the cache forgets that folder so the next rename makes it again. The next sync starts with an empty cache. | + +Kept as they were: the pre-publish `stat(path)` (a new file must still be +absent), both verifies (the temporary before publishing, the path after), +the original's verify before it moves aside, and every rollback. + +The fifth call the Android shell dropped, a `Filesystem.getUri` before every +`inspect`, is not here to drop: `NativeFs.readForSync` already builds a local +vault's file URIs from the root resolved at open. An iCloud vault still +materializes and stats each file before reading it, on purpose, so an +evicted `.icloud` stub is never fingerprinted in place of the note. + +One stricter behavior: a staged publish (large or referenced files) whose +rollback copy would not delete now stops the next write in the same run +with "Cloud sync needs recovery". Before, the run went on and the next sync's +scan stopped at the same file, so the user lands in the same place one step +sooner, the same way an inline write's leftover backup already behaved. + +### iCloud vaults + +Not exercised: the scratch simulator had no iCloud account. An iCloud vault +writes through the same Capacitor `Filesystem` calls, with file URLs in the +ubiquity container instead of a Documents path, so the two rename facts above +hold there as well. Two things differ only there: + +- `.zennotes/sync` sits inside the synced folder, so another device's leftover + rollback file can arrive through iCloud during a run. Before, the next write + would have stopped on it; now the next run's first write or scan does. That + file records the other device's interrupted write, not one of this run's, + so nothing this run does depends on seeing it sooner. +- A folder deleted by another device mid-run makes the next rename into it + fail before it changes anything. That write rolls back like any failed + write and the folder is made again on the next rename. Before, the `mkdir` + in front of every rename would have put the folder back first. + +## Results + +iOS Simulator (iOS 26.5, iPhone 17 Pro, a scratch device made for this and +deleted afterwards), fake Cloud fixture on the Mac, small inline notes in four +folders, a local vault ("On this iPhone"). Fresh install each run (uninstall, +install, onboard, link), bridge calls counted by wrapping +`Capacitor.nativePromise`. Before and after apps were built from this +worktree, differing only in the two source files, and run alternately. + +The Mac was busy with other work throughout (load average 57 to 116 on 16 +cores), which moved single runs by up to 2x in either direction, so the table +gives medians over every run: + +| Run | Before | After | Change | +|---|---|---|---| +| First sync, 600 notes (5 runs each) | 4.34 s, 138 notes/s | 3.49 s, 172 notes/s | 20% less time | +| First sync, 2,500 notes (3 runs each) | 17.24 s, 145 notes/s | 14.66 s, 171 notes/s | 15% less time | + +All runs: 600 before 4.34, 6.51, 4.23, 4.20, 8.56 s and after 3.39, 3.49, +3.36, 8.13, 3.99 s; 2,500 before 17.01, 17.24, 17.92 s and after 14.66, +17.36, 14.36 s. + +Bridge calls per pulled file, first sync of 600 (identical in every run): + +| Call | Before | After | +|---|---|---| +| `Filesystem.stat` | 5.02 | 4.02 | +| `ZenDirectUpload.inspect` | 3.00 | 3.00 | +| `Filesystem.readdir` | 1.05 | 0.05 | +| `Filesystem.readFile` | 1.01 | 1.01 | +| `Filesystem.writeFile` | 1.00 | 1.00 | +| `Filesystem.mkdir` | 1.00 | 0.01 | +| `Filesystem.rename` | 1.00 | 1.00 | +| `Filesystem.deleteFile` | 1.00 | 0 | +| **Total** | **14.11** | **10.12** | + +At 2,500 notes it went from 14.03 to 10.03. A call costs about 0.3 to 0.9 ms +on the simulator (`inspect` the most), cheaper than the Android emulator's +1 ms, so the share of time saved is the number to carry to a real phone, not +the seconds. + +Same results: the vault's files hashed identically (SHA-256 of every file +outside `.zennotes`) between before and after for every pair: 602 files at +600 and 2,502 at 2,500. `.zennotes/sync` held nothing but an empty +`downloads` folder after every run. With the Welcome note served as a content +reference, resolving the first-sync conflict with Cloud's version (a staged +download and publish over an existing note) landed the other device's text, +left no conflict pending and no rollback file. + +## What is left per file + +About 10 calls: `stat` 4 (core's existence check, the write's own, the +pre-publish check, the post-sync scan's stability check), `inspect` 3 (both +verifies, and the scan reading the new file), and one each of `writeFile`, +`rename` and `readFile` (the app's rescan after sync). The core's check and +the write's own `stat` look alike but sit on either side of the core +boundary; merging them is a core API change. + +## Tests + +`npm test`: 295 pass (289 before, 6 new). `npm run typecheck` and +`npm run boundaries:check` clean. + +- `cloud-sync-repository.test.ts`, "bridge calls per pulled file": the exact + call list for a new file (one recovery listing, no backup delete, no + temporary check); a replaced original is still verified, moved aside and its + rollback copy deleted; a rollback copy that will not delete stops the next + write; a failed write lists recovery files again; a staged publish that kept + its rollback copy stops the next write. +- `mobile-cloud-sync.integration.test.ts`: each folder is made once per sync, + and again on the next sync. + +Against the old code, the call-count tests fail, as does the staged-publish +one (it pins the stricter behavior above); the other recovery tests pass on +both. + +## How to test locally + +1. `npm test`, `npm run typecheck` and `npm run boundaries:check` in this + checkout. +2. Run a debug build on a simulator with a Cloud vault linked (for the fake + Cloud, build with `VITE_ZENNOTES_CLOUD_DEV_URL=http://127.0.0.1:`). + Debug builds are inspectable: Safari, Develop, the simulator, ZenNotes. + In the console, count calls during a sync: + + ```js + const cap = window.Capacitor, stats = {} + cap.__orig ??= cap.nativePromise + cap.nativePromise = function (plugin, method, options) { + const s = (stats[`${plugin}.${method}`] ??= { n: 0 }) + s.n++ + return cap.__orig.call(this, plugin, method, options) + } + const summary = await window.zen.syncCloudVault() + Object.entries(stats).map(([k, v]) => `${k} ${(v.n / summary.pulled).toFixed(2)}`) + ``` + + A sync that pulls new notes should show no `deleteFile`, almost no `mkdir` + or `readdir`, and about 10 calls per file in total. +3. Pull a note into a new folder, edit a pulled note on another device, then + sync: both land. Resolve a conflict with Cloud's version. Afterwards + `.zennotes/sync` holds nothing but an empty `downloads` folder. diff --git a/src/bridge/cloud-sync-repository.test.ts b/src/bridge/cloud-sync-repository.test.ts index 29403a7..cf28d39 100644 --- a/src/bridge/cloud-sync-repository.test.ts +++ b/src/bridge/cloud-sync-repository.test.ts @@ -644,3 +644,131 @@ describe('cached repository without crypto.randomUUID (iOS 15.0 to 15.3)', () => assert.deepEqual([...h.files.keys()], ['note.md']) }) }) + +describe('bridge calls per pulled file', () => { + const ID = /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/g + /** The repository over call-logging copies of the harness's fs and native files. */ + function logged(h: ReturnType, extra: Record = {}) { + const calls: string[] = [] + const log = >(target: T) => Object.fromEntries(Object.entries(target).map( + ([name, value]) => [name, typeof value !== 'function' ? value : (...args: unknown[]) => { + calls.push([name, ...args.filter((arg) => typeof arg === 'string')].join(' ').replace(ID, '')) + return value(...args) + }] + )) as T + const repository = new CachedCloudSyncRepository(log(h.fs), log({ ...h.native, ...extra }), h.store) + return { calls, repository } + } + const upsert = (sequence: number, path: string, text: string) => ({ + sequence, item_id: `item-${sequence}`, revision: 1, type: 'upsert' as const, path, previous_path: null, content: content(text) + }) + const tracked = (path: string, text: string) => ({ + item_id: 'item-0', path, kind: 'text' as const, revision: 1, + sha256: content(text).sha256, byte_length: Buffer.byteLength(text), media_type: 'text/markdown' + }) + + it('lists recovery files once and skips backup and temporary cleanup for new files', async () => { + const h = harness({}) + const { calls, repository } = logged(h) + await repository.apply(upsert(1, 'a.md', 'A'), undefined) + const first = calls.splice(0) + await repository.apply(upsert(2, 'folder/b.md', 'B'), undefined) + await repository.apply(upsert(3, 'folder/c.md', 'C'), undefined) + + assert.deepEqual(first, [ + 'stat a.md', + 'readdirStrict .zennotes/sync', + 'stat a.md', + 'writeText .zennotes/sync/write-.md A', + 'readForSync .zennotes/sync/write-.md', + 'stat a.md', + 'rename .zennotes/sync/write-.md a.md', + 'readForSync a.md' + ]) + assert.equal(calls.filter((call) => call.startsWith('readdirStrict')).length, 0) + assert.equal(calls.filter((call) => call.startsWith('deleteFile')).length, 0) + assert.equal(calls.filter((call) => call.startsWith('stat .zennotes')).length, 0) + assert.deepEqual([...h.files.keys()].sort(), ['a.md', 'folder/b.md', 'folder/c.md']) + assert.equal(h.files.get('folder/c.md')?.bytes.toString(), 'C') + }) + + it('still verifies a replaced original and removes its rollback copy', async () => { + const h = harness({ 'note.md': 'Old' }) + const { calls, repository } = logged(h) + await repository.apply({ ...upsert(2, 'note.md', 'New'), item_id: 'item-0', revision: 2 }, tracked('note.md', 'Old')) + assert.equal(h.files.get('note.md')?.bytes.toString(), 'New') + assert.deepEqual([...h.files.keys()], ['note.md']) + assert.deepEqual(calls.filter((call) => /^(rename|deleteFile)/.test(call)), [ + 'rename note.md .zennotes/sync/rollback-.md', + 'rename .zennotes/sync/write-.md note.md', + 'deleteFile .zennotes/sync/rollback-.md' + ]) + // The original is verified before it moves aside, the new bytes after they land. + const aside = calls.indexOf('rename note.md .zennotes/sync/rollback-.md') + const landed = calls.indexOf('rename .zennotes/sync/write-.md note.md') + assert.equal(calls[aside - 1], 'readForSync note.md') + assert.equal(calls[landed + 1], 'readForSync note.md') + }) + + it('stops the next write when a rollback copy would not delete', async () => { + const h = harness({ 'note.md': 'Old' }) + const deleteFile = h.fs.deleteFile + h.fs.deleteFile = async (path: string) => { + if (path.includes('/rollback-')) throw new Error('Delete refused') + await deleteFile(path) + } + const repository = new CachedCloudSyncRepository(h.fs, h.native, h.store) + await repository.apply({ ...upsert(2, 'note.md', 'New'), item_id: 'item-0', revision: 2 }, tracked('note.md', 'Old')) + const rollback = [...h.files.keys()].find((path) => path.includes('/rollback-')) + assert.equal(h.files.get(rollback!)?.bytes.toString(), 'Old') + await assert.rejects(repository.apply(upsert(3, 'other.md', 'Other'), undefined), /needs recovery/) + assert.equal(h.files.has('other.md'), false) + }) + + it('lists recovery files again after a failed write', async () => { + const h = harness({}) + const { calls, repository } = logged(h) + h.setWriteHook((path) => { + h.setWriteHook(undefined) + throw new Error(`Native write failed for ${path}`) + }) + await assert.rejects(repository.apply(upsert(1, 'a.md', 'A'), undefined), /Native write failed/) + await repository.apply(upsert(2, 'b.md', 'B'), undefined) + assert.equal(calls.filter((call) => call === 'readdirStrict .zennotes/sync').length, 2) + assert.deepEqual([...h.files.keys()], ['b.md']) + }) + + it('stops the next write when a staged publish kept its rollback copy', async () => { + const h = harness({ 'note.md': 'Old' }) + const cloud = Buffer.from('Cloud version') + const ref = { + item_id: 'item-0', revision: 2, encoding: 'utf8', sha256: createHash('sha256').update(cloud).digest('hex'), + byte_length: cloud.length, media_type: 'text/markdown' + } + const { repository } = logged(h, { + statVerified: async (path: string) => (await h.fs.stat(path)), + download: async (options: { to: string }) => { h.put(options.to, cloud) }, + rename: h.fs.rename, + deleteFile: async (path: string) => { + if (path.includes('/rollback-')) throw new Error('Delete refused') + await h.fs.deleteFile(path) + }, + mkdir: async () => {}, + readText: async (path: string) => h.files.get(path)!.bytes.toString() + }) + const file = await repository.stageCloudContent!({ + reference: ref, previewLimitBytes: 262_144, allowInsecureLoopback: false, + getInstruction: async () => ({ + item_id: ref.item_id, revision: ref.revision, + content: { encoding: ref.encoding, sha256: ref.sha256, byte_length: ref.byte_length, media_type: ref.media_type }, + download: { url: 'https://objects.example.test/item-0/2', method: 'GET', headers: {}, + expires_at: new Date(Date.now() + 300_000).toISOString() } + }) + }) + await repository.applyStagedCloudContent!({ sequence: 2, item_id: 'item-0', type: 'upsert', path: 'note.md', + previous_path: null, revision: 2, content_ref: ref }, tracked('note.md', 'Old'), file) + assert.equal(h.files.get('note.md')?.bytes.toString(), 'Cloud version') + await assert.rejects(repository.apply(upsert(3, 'other.md', 'Other'), undefined), /needs recovery/) + assert.equal(h.files.has('other.md'), false) + }) +}) diff --git a/src/bridge/cloud-sync-repository.ts b/src/bridge/cloud-sync-repository.ts index eea66fc..97ab6d8 100644 --- a/src/bridge/cloud-sync-repository.ts +++ b/src/bridge/cloud-sync-repository.ts @@ -81,8 +81,14 @@ export class CachedCloudSyncRepository extends PortableCloudSyncRepository { await contentFiles.assertReady(true) return staging.stage(source) }, - applyStagedCloudContent: (...args: Parameters) => staging.apply(...args), - resolveStagedCloudConflict: (input: Parameters[0]) => staging.resolve(input) + // A staged publish keeps its own rollback copy, so the next read or + // write looks for one again. + applyStagedCloudContent: async (...args: Parameters) => { + try { return await staging.apply(...args) } finally { contentFiles.recheckRecovery() } + }, + resolveStagedCloudConflict: async (input: Parameters[0]) => { + try { return await staging.resolve(input) } finally { contentFiles.recheckRecovery() } + } } : {}), readdir: (path: string) => fs.readdir(path), stat: (path: string) => fs.stat(path), @@ -207,6 +213,11 @@ class NativeCloudSyncContent { this.recoveryChecked = true } + /** Something may have left a rollback file since the last listing. */ + recheckRecovery(): void { + this.recoveryChecked = false + } + async read(path: string, uri?: string): Promise { await this.assertReady() const file = await this.native.readForSync(path, TEXT_EXTENSIONS.has(extension(path)), uri) @@ -268,6 +279,7 @@ class NativeCloudSyncContent { const temporary = `.zennotes/sync/write-${randomUUID()}${extension(path)}` const backup = `.zennotes/sync/rollback-${randomUUID()}${extension(path)}` let publishing = false + let published = false let completed = false try { const source = this.sources.get(content) @@ -288,6 +300,7 @@ class NativeCloudSyncContent { } publishing = true await this.fs.rename(temporary, path) + published = true await this.verify(path, content) completed = true } catch (error) { @@ -305,9 +318,13 @@ class NativeCloudSyncContent { } throw error } finally { - this.recoveryChecked = false - if (await this.fs.stat(temporary) === 'file') await this.fs.deleteFile(temporary).catch(() => {}) - if (completed) await this.fs.deleteFile(backup).catch(() => {}) + // A write that completed and removed its backup leaves no rollback + // file, so the listing it started from still holds. + if (!completed) this.recoveryChecked = false + // A rename that resolved moved the temporary onto the path. + if (!published && await this.fs.stat(temporary) === 'file') await this.fs.deleteFile(temporary).catch(() => {}) + // There is a backup only when an original was moved aside. + if (completed && before) await this.fs.deleteFile(backup).catch(() => { this.recoveryChecked = false }) } } } diff --git a/src/bridge/mobile-cloud-sync.integration.test.ts b/src/bridge/mobile-cloud-sync.integration.test.ts index 7217421..f4f4962 100644 --- a/src/bridge/mobile-cloud-sync.integration.test.ts +++ b/src/bridge/mobile-cloud-sync.integration.test.ts @@ -232,6 +232,24 @@ describe('mobile Cloud adapter wiring', () => { }) } + it('makes each folder once per sync however many pulled files land in it', async () => { + const h = await fixture({}) + for (const path of ['notes/a.md', 'notes/b.md', 'notes/deeper/c.md', 'notes/d.md', 'top.md']) { + h.remoteText(path, `Content of ${path}`) + } + await h.sync() + assert.deepEqual([...h.files.keys()].sort(), ['notes/a.md', 'notes/b.md', 'notes/d.md', 'notes/deeper/c.md', 'top.md']) + assert.deepEqual(h.createdDirectories, ['notes', 'notes/deeper']) + // A folder removed between syncs comes back: each sync asks again. The + // edit's original moves aside into .zennotes/sync before the new bytes land. + h.createdDirectories.length = 0 + h.remoteText('notes/e.md', 'Fifth note') + h.remoteText('notes/b.md', 'Edited on desktop') + await h.sync() + assert.deepEqual(h.createdDirectories, ['notes', '.zennotes/sync']) + assert.equal(h.files.get('notes/b.md')?.bytes.toString(), 'Edited on desktop') + }) + it('detects remote changes from a one-item metadata manifest without scanning local files', async () => { const h = await fixture() assert.equal(await h.api.hasMobileCloudVaultChanges(h.vault), true) diff --git a/src/bridge/mobile-cloud-sync.ts b/src/bridge/mobile-cloud-sync.ts index d0defee..e6b69ea 100644 --- a/src/bridge/mobile-cloud-sync.ts +++ b/src/bridge/mobile-cloud-sync.ts @@ -357,6 +357,9 @@ async function linksEncryptedVault( } function hostVault(vault: MobileVault, cacheScan = false): CloudSyncHostVault { + // Folders this operation has made, so a pull asks for each one once rather + // than before every file it renames into place. + const folders = new Set() const fs: PortableCloudSyncFileSystem = { // Strict on purpose: NativeFs.readdir maps failure to [], which scan() // would read as an empty vault and plan a delete for every tracked item. @@ -376,8 +379,18 @@ function hostVault(vault: MobileVault, cacheScan = false): CloudSyncHostVault { // A pulled move carries no write that would create the destination // folder, and Capacitor's rename fails on a missing parent. const parent = to.includes('/') ? to.slice(0, to.lastIndexOf('/')) : '' - if (parent) await vault.fs.mkdir(parent) - await vault.fs.rename(from, to) + if (parent && !folders.has(parent)) { + await vault.fs.mkdir(parent) + folders.add(parent) + } + try { + await vault.fs.rename(from, to) + } catch (error) { + // The folder may have gone since: a rename into a missing folder + // fails before it changes anything, and the next one makes it again. + folders.delete(parent) + throw error + } } }