Repository navigation
perf: pre-build MutexStorageBackend in CommittedFactLoaderImpl to eliminate per-resolve() Arc::clone (#280) - #281
Merged
Conversation
…280 regression #279 fixed concurrent mutex serialization by switching resolve() to use MutexStorageBackend instead of locking backend directly before get_or_load(). This kept #279's correctness (mutex only acquired on cache misses) but introduced an Arc::clone + Arc::drop on every resolve() call. At 10k facts, point_attribute queries call resolve() ~10k times per iteration, making the per-call overhead measurable: +22% on query/point_attribute/10k (29.4 ms vs 24.1 ms pre-regression), with cascading slowdowns of 8-17% on other large-dataset query benchmarks. Fix: construct MutexStorageBackend once at CommittedFactLoaderImpl creation time and store it as a field. resolve() now uses &self.backend_adapter directly — zero per-call allocation or atomic ops. The concurrency property is fully preserved: backend mutex is still only acquired inside MutexStorageBackend::read_page() on a cache miss. Closes #280 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
adityamukho
added a commit
that referenced
this pull request
May 21, 2026
- BENCHMARKS.md: replace stale v0.8.0 query latency entries for point_attribute and join_3pattern with v1.1.1 Bencher CI baseline (ubuntu-latest); add predicate_pushdown section; update regex_filter estimates to actual CI numbers; fix stale prose claiming selective B+tree lookup is a post-1.0 backlog item (shipped in #208); add Post-1.0 Updates header documenting #208/#279/#281 impact on numbers - README.md: fix stale "(Phase 7 target: predicate pushdown)" note — predicate pushdown shipped in #207 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
resolve()by usingMutexStorageBackendinstead of locking the backend beforeget_or_load(). However, it createdMutexStorageBackend(Arc::clone(&self.backend))on everyresolve()call — at 10k facts, that's 10kArc::clone+ 10kArc::droppairs per query iteration.query/point_attribute/10kwent from 24.117 ms (May 18, pre-perf: fix backend mutex serialisation in CommittedFactLoaderImpl::resolve (#252) #279) to 29.374 ms (May 20, post-perf: fix backend mutex serialisation in CommittedFactLoaderImpl::resolve (#252) #279) — +22%. Other large-dataset benchmarks regressed 8–17%. The 1k variants were unaffected, confirming the cost is per-call and proportional to fact count.backend_adapter: MutexStorageBackend<B>as a struct field onCommittedFactLoaderImpl, constructed once (oneArc::cloneat init time).resolve()uses&self.backend_adapterdirectly — zero per-call overhead.MutexStorageBackendstill only acquires the backend mutex insideread_page()on a cache miss. Concurrent readers are not serialized on cache hits.Test plan
cargo test)cargo clippycleancargo fmt --checkcleanCommittedFactLoaderImplconstruction sites updatedCloses #280
🤖 Generated with Claude Code