refactor: extract file caching into CachingBackend wrapper#7
refactor: extract file caching into CachingBackend wrapper#7jacksonrnewhouse wants to merge 1 commit intomarkovejnovic/some-decent-performancefrom
Conversation
Move file caching logic from SsFs into a separate CachingBackend<B> that implements SsfsBackend by wrapping an inner backend. This provides better separation of concerns - SsFs handles inode/filesystem logic while CachingBackend handles LRU caching. Key changes: - Add CachingBackend<B: SsfsBackend> with LRU file caching - Remove caching fields and logic from SsFs - Update MesaFS to wrap MesaBackend with CachingBackend - Simplify tests to use the new wrapper pattern Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Mesa DescriptionMove file caching logic from SsFs into a separate CachingBackend that implements SsfsBackend by wrapping an inner backend. This provides better separation of concerns - SsFs handles inode/filesystem logic while CachingBackend handles LRU caching. Key changes:
The language models get upset about a race condition, where the backend could be asked to fetch the same file multiple times. This doesn't happen in practice as instructions from /dev/fuse are handled sequentially. Description generated by Mesa. Update settings |
There was a problem hiding this comment.
Performed full review of 4924bf5...fa2739e
Analysis
- The refactoring sacrifices performance for cleaner separation of concerns - moving caching behind the SsfsBackend trait has increased steady-state read latency and backend load
- SsFs no longer differentiates between resolved vs. asynchronous reads after the architectural changes
- The system has lost the ability to deduplicate concurrent fetches, potentially causing redundant backend operations
- Future work needed: extend the backend abstraction to restore these performance guarantees without compromising the new separation of concerns
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
0 files reviewed | 2 comments | Edit Agent Settings • Read Docs
| entries.push((*ino, entry.last_access(), entry.data.len() as u64)); | ||
| true // continue iterating | ||
| let backend = Arc::clone(&self.backend); | ||
| Ok(SsfsOk::Future(Box::pin(async move { |
There was a problem hiding this comment.
With the caching logic pushed behind SsfsBackend, SsFs::read now always returns SsfsOk::Future, even when the data is already fully resident in memory. The previous implementation returned SsfsOk::Resolved on a cache hit, which let the FUSE call path stay synchronous (no rt.block_on, no async context switching). This change adds an unconditional tokio hop to every read, effectively erasing the low‑latency fast path that made hot files cheap to serve. Could we extend the backend trait with a synchronous "try_read_cached" or similar hook so a warm cache can still be returned as Resolved? Otherwise we're regressing steady‑state read latency for no functional gain.
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: mesa-dot-dev/git-fs#7
File: crates/git-fs/src/ssfs.rs#L842
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
With the caching logic pushed behind `SsfsBackend`, `SsFs::read` now always returns `SsfsOk::Future`, even when the data is already fully resident in memory. The previous implementation returned `SsfsOk::Resolved` on a cache hit, which let the FUSE call path stay synchronous (no `rt.block_on`, no async context switching). This change adds an unconditional tokio hop to every read, effectively erasing the low‑latency fast path that made hot files cheap to serve. Could we extend the backend trait with a synchronous "try_read_cached" or similar hook so a warm cache can still be returned as `Resolved`? Otherwise we're regressing steady‑state read latency for no functional gain.
|
|
||
| debug!(path = %path_owned, "cache miss, fetching from backend"); | ||
|
|
||
| let result = inner.read_file(&path_owned).await; |
There was a problem hiding this comment.
In the old SsFs implementation we kept a pending_file_fetches map and had readers share the same in‑flight fetch so the backend only saw one request per inode. That coordination disappeared in this wrapper: cache misses immediately call inner.read_file, so two overlapping reads on the same path will both go to the backend until the first response lands in the cache. Given the GitHub API’s rate limits, this duplication can get expensive. Can we restore per‑path fetch deduplication inside CachingBackend (e.g., store an Arc<Notify> or Future alongside the cache entry) so concurrent miss traffic remains coalesced like before?
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: mesa-dot-dev/git-fs#7
File: crates/git-fs/src/ssfs.rs#L291
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
In the old `SsFs` implementation we kept a `pending_file_fetches` map and had readers share the same in‑flight fetch so the backend only saw one request per inode. That coordination disappeared in this wrapper: cache misses immediately call `inner.read_file`, so two overlapping reads on the same path will both go to the backend until the first response lands in the cache. Given the GitHub API’s rate limits, this duplication can get expensive. Can we restore per‑path fetch deduplication inside `CachingBackend` (e.g., store an `Arc<Notify>` or `Future` alongside the cache entry) so concurrent miss traffic remains coalesced like before?
Move file caching logic from SsFs into a separate
CachingBackend<B>that implements SsfsBackend by wrapping an inner backend. This provides better separation of concerns - SsFs handles inode/filesystem logic whileCachingBackendhandles LRU caching.Key changes:
The language models get upset about a race condition, where the backend could be asked to fetch the same file multiple times. This doesn't happen in practice as instructions from /dev/fuse are handled sequentially.