perf(sync): reuse opened mirrors, barrier fsync, single-open staging - #71
Open
srnnkls wants to merge 3 commits into
Open
perf(sync): reuse opened mirrors, barrier fsync, single-open staging#71srnnkls wants to merge 3 commits into
srnnkls wants to merge 3 commits into
Conversation
Cold deploy re-opened the gix mirror and re-validated every pack index for each of the 2,349 leaf reads; GitBackend now keeps one ThreadSafeRepository per mirror and forgets it when the mirror is refreshed or a worktree is recaptured. Journal and registry writes used sync_all, which is F_FULLFSYNC on macOS (5.7 ms each here, 4 per artifact); write-then-rename only needs ordering, so they use F_BARRIERFSYNC (0.7 ms). Staged leaves are written, chmodded and timestamped through one open handle instead of write + reopen for mtime + metadata/set_permissions. arvato feat-phora target (94 artifacts, 2,349 leaves), hyperfine: cold deploy 5.95 s -> 1.79 s, rebuild-registry 4.44 s -> 1.30 s, sync --force 4.19 s -> 2.11 s, noop unchanged (~90 ms).
There was a problem hiding this comment.
🟡 Changes recommended
The new staging mtime path can panic on overflow (UNIX_EPOCH + Duration::from_secs(commit_time)), which should be converted back into a handled error as before.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR optimizes sync/deploy performance by reducing repeated Git mirror opens, lowering macOS fsync overhead during write-then-rename persistence, and avoiding extra file reopens when staging leaf outputs.
Changes:
- Cache opened
gix::ThreadSafeRepositoryinstances per mirror inGitBackend, handing out thread-local repositories and invalidating the cache on refresh/recapture paths. - Introduce
fsync_barrierto avoidF_FULLFSYNCoverhead on macOS while preserving write ordering for atomic writes. - Write/chmod/timestamp staged leaf files through a single open file handle.
File summaries
| File | Description |
|---|---|
| src/sync/state/mod.rs | Adds durable fsync module and re-exports fsync_barrier for state writers. |
| src/sync/state/file.rs | Uses fsync_barrier in atomic state record writes to reduce macOS overhead. |
| src/sync/state/durable.rs | Implements fsync_barrier (macOS barrier + fallback; other OS uses sync_all) and adds a unit test. |
| src/sync/journal.rs | Uses fsync_barrier for journal temp-file durability step before rename. |
| src/sync/stage.rs | Consolidates leaf staging into a single-open write_leaf flow including exec bit + mtime setting. |
| src/source/git.rs | Caches opened mirrors in GitBackend and invalidates the cache on mirror refresh / worktree resolution paths. |
| src/source/mod.rs | Adds regression test asserting correct cache invalidation across refresh and reclone scenarios. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| file.set_permissions(perms)?; | ||
| } | ||
| } | ||
| file.set_modified(std::time::UNIX_EPOCH + std::time::Duration::from_secs(commit_time))?; |
Owner
Author
There was a problem hiding this comment.
Fixed in a986d07: checked_add and the original commit_time out of range error.
Comment on lines
+4
to
+6
| /// Rust std maps `sync_all` to `F_FULLFSYNC` on Apple targets; write-then-rename | ||
| /// only needs the ordering `F_BARRIERFSYNC` gives. | ||
| #[cfg(target_os = "macos")] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cold deploy,
rebuild-registry, andsync --forcespent most of their time re-opening the gix mirror per leaf and inF_FULLFSYNC. Three changes:GitBackendkeeps oneThreadSafeRepositoryper mirror and hands outto_thread_local()handles; the entry is forgotten when the mirror is refreshed or a worktree is recaptured. Before, everyreadrangix::openand re-validated every pack index — 2,349 times per cold deploy.sync_all, which Rust maps toF_FULLFSYNCon Apple targets (5.7 ms each here, 4 per artifact). They only need ordering, so macOS now usesF_BARRIERFSYNC(0.7 ms) viasync::state::fsync_barrier; other targets keepsync_all. Durability on power loss is weaker; intent-before-swap ordering is unchanged.fs::write+ afiletimereopen +metadata/set_permissions.Measurements
arvato
feat/phoratarget (94 artifacts, 2,349 leaves), hyperfine, macOS, disk 96 % full so cold-path variance is high:rebuild-registrysync --forcesyncCPU time (user+sys) drops 2.6× on every path; the remainder is APFS file creation and blob inflate.
Tests
reads_after_refresh_see_the_new_commit_and_the_old_snapshotpins cache invalidation across fetch and reclone.barrier_succeeds_on_a_written_filecovers the barrier call.cargo test: 1,434 lib + 57 integration pass; clippy-D warningsand fmt clean.Independent of #70 (different files); both branch from
main.