From b0caed5a5bb44837dc8201d35991c6b0dea8bd05 Mon Sep 17 00:00:00 2001 From: Aditya Mukhopadhyay Date: Thu, 21 May 2026 08:11:43 +0530 Subject: [PATCH 1/2] perf: pre-build MutexStorageBackend in CommittedFactLoaderImpl to fix #280 regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 --- src/storage/persistent_facts.rs | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/storage/persistent_facts.rs b/src/storage/persistent_facts.rs index 7196d8d37..45c45ab2f 100644 --- a/src/storage/persistent_facts.rs +++ b/src/storage/persistent_facts.rs @@ -46,12 +46,19 @@ fn compute_index_checksum(facts: &[Fact]) -> u32 { /// Resolves FactRefs to Fact objects by reading packed pages from the backend /// through the page cache. Used after loading (or migrating) a v5/v6 file so that indexes can /// resolve committed facts without keeping the entire fact list in memory. -// page_cache is read in the CommittedFactReader::resolve impl; Rust's dead-code -// lint does not track trait-impl field reads when the impl is behind dyn dispatch. +// Fields are read inside CommittedFactReader trait methods that are accessed via +// dyn dispatch — Rust's dead-code lint does not track trait-impl field reads +// when the impl is behind dyn dispatch. struct CommittedFactLoaderImpl { + #[allow(dead_code)] backend: Arc>, #[allow(dead_code)] page_cache: Arc, + /// Pre-built adapter reused on every `resolve()` call. + /// Avoids an `Arc::clone` per call: the adapter is constructed once and + /// holds the `Arc>` for the lifetime of this reader. + /// Backend mutex is still only acquired on cache misses (see `MutexStorageBackend`). + backend_adapter: MutexStorageBackend, committed_fact_pages: Arc, #[allow(dead_code)] first_fact_page: u64, // always 1 in current layout @@ -64,12 +71,9 @@ impl crate::storage::CommittedFactReader &self, fact_ref: crate::storage::index::FactRef, ) -> anyhow::Result { - // Use MutexStorageBackend so the backend mutex is acquired only when - // get_or_load actually needs to read a cold page from disk. On cache - // hits get_or_load returns without ever calling read_page, so no lock - // is acquired and concurrent readers do not serialize. - let adapter = MutexStorageBackend(Arc::clone(&self.backend)); - let page = self.page_cache.get_or_load(fact_ref.page_id, &adapter)?; + // backend_adapter is pre-built at construction time — no Arc::clone per call. + // Backend mutex is only acquired inside adapter.read_page() on a cache miss. + let page = self.page_cache.get_or_load(fact_ref.page_id, &self.backend_adapter)?; crate::storage::packed_pages::read_slot(&page, fact_ref.slot_index) } @@ -295,6 +299,7 @@ impl PersistentFactStorage { let loader: std::sync::Arc = std::sync::Arc::new(CommittedFactLoaderImpl { backend: self.backend.clone(), + backend_adapter: MutexStorageBackend(self.backend.clone()), page_cache: self.page_cache.clone(), committed_fact_pages: self.committed_fact_pages.clone(), first_fact_page: 1, @@ -781,6 +786,7 @@ impl PersistentFactStorage { let loader: Arc = Arc::new(CommittedFactLoaderImpl { backend: self.backend.clone(), + backend_adapter: MutexStorageBackend(self.backend.clone()), page_cache: self.page_cache.clone(), committed_fact_pages: self.committed_fact_pages.clone(), first_fact_page: 1, @@ -990,6 +996,7 @@ impl PersistentFactStorage { let loader: Arc = Arc::new(CommittedFactLoaderImpl { backend: self.backend.clone(), + backend_adapter: MutexStorageBackend(self.backend.clone()), page_cache: self.page_cache.clone(), committed_fact_pages: self.committed_fact_pages.clone(), first_fact_page: 1, From 5a693810aff2a6017694f7d7bb0a2f7cb7086572 Mon Sep 17 00:00:00 2001 From: Aditya Mukhopadhyay Date: Thu, 21 May 2026 08:11:54 +0530 Subject: [PATCH 2/2] style: apply rustfmt Co-Authored-By: Claude Sonnet 4.6 --- src/storage/persistent_facts.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/storage/persistent_facts.rs b/src/storage/persistent_facts.rs index 45c45ab2f..800b0942f 100644 --- a/src/storage/persistent_facts.rs +++ b/src/storage/persistent_facts.rs @@ -73,7 +73,9 @@ impl crate::storage::CommittedFactReader ) -> anyhow::Result { // backend_adapter is pre-built at construction time — no Arc::clone per call. // Backend mutex is only acquired inside adapter.read_page() on a cache miss. - let page = self.page_cache.get_or_load(fact_ref.page_id, &self.backend_adapter)?; + let page = self + .page_cache + .get_or_load(fact_ref.page_id, &self.backend_adapter)?; crate::storage::packed_pages::read_slot(&page, fact_ref.slot_index) }