Repository navigation
perf: fix backend mutex serialisation in CommittedFactLoaderImpl::resolve (#252) - #279
Merged
Merged
Conversation
…erImpl::resolve CommittedFactLoaderImpl::resolve was acquiring the Arc<Mutex<B>> backend lock before calling page_cache.get_or_load(), so even cache hits (which never call read_page) serialised concurrent readers on the mutex. Under concurrent load (e.g. the concurrent_btree_scan benchmark), all threads queued behind one another even when every page was already warm in the LRU cache. Fix: use MutexStorageBackend (promoted to pub(crate)) so the backend mutex is acquired only if get_or_load has a cache miss and actually calls read_page — identical to the pattern already used by OnDiskIndexReader::range_scan_*. Result on concurrent_btree_scan (local, post-fix vs CI regression): 10k/4t: 45ms → 31ms (+43% throughput) 10k/8t: 70ms → 57ms (+23% throughput) 100k/8t: 2.1s → 1.48s (+30% throughput, now correctly above 100k/4t) Closes #252 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
4 tasks done
adityamukho
added a commit
that referenced
this pull request
May 21, 2026
…minate per-resolve() Arc::clone (#280) (#281) * perf: pre-build MutexStorageBackend in CommittedFactLoaderImpl to fix #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> * style: apply rustfmt Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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
CommittedFactLoaderImpl::resolveacquiredArc<Mutex<B>>before callingpage_cache.get_or_load(), so even warm-cache hits (which never callread_page) serialised all concurrent readers on one mutexdaa80a9(selective B+tree lookup, perf(post-1.0): B+Tree selective lookup for bound patterns #208/feat: selective B+Tree lookup in filter_facts_for_query (#208) #246) caused the query[:e0 :val ?v]to callget_facts_by_attribute(\":val\")returning all n FactRefs, each needing aresolve()call — 10k/100k mutex acquisitions vs the previous singlestream_all()acquisitionMutexStorageBackendtopub(crate)and use it inresolve(), matching the pattern already used byOnDiskIndexReader::range_scan_*— backend mutex is now held only for actual cold-page readsBenchmark results (local, fixed vs regressed CI)
Test plan
cargo clippycleancargo fmt --checkcleanconcurrent_btree_scanbenchmarks show improved throughput scaling with thread countCloses #252
🤖 Generated with Claude Code