From 225c2a751aab841564c873201e2a30027826d6a9 Mon Sep 17 00:00:00 2001 From: Aditya Mukhopadhyay Date: Sun, 23 Aug 2026 22:21:07 +0530 Subject: [PATCH] perf: skip redundant page cache allocation in BrowserDb (#275) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BrowserDb::open_in_memory(), open(), and import_graph() now pass page cache capacity 0 to PersistentFactStorage::new instead of 256. BrowserBufferBackend already holds every page resident in a HashMap (pre-loaded from IndexedDB at open time), so the LRU PageCache on top of it was a redundant RAM-to-RAM copy plus bookkeeping on every read. This required fixing PageCache::new(0) itself: it previously meant "unbounded" rather than "disabled" (the eviction loop's capacity > 0 guard skipped eviction, but insertion still ran unconditionally), which would have made this change strictly worse — an ever-growing duplicate of every page ever read, never reclaimed. Capacity 0 now disables the cache outright: get_or_load reads through to the backend and put_dirty is a no-op. No existing caller passed 0 before, so no other behaviour changes. Added PageCache::capacity() and a wasm+test-only PersistentFactStorage::page_cache_capacity() accessor so the new BrowserDb tests can assert the constructors wire this up correctly. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01YMWSutMLJJ7Y5b8hs9ZaCU --- CHANGELOG.md | 6 ++++ src/browser/mod.rs | 45 ++++++++++++++++++++++++++---- src/storage/cache.rs | 49 +++++++++++++++++++++++++++++++++ src/storage/persistent_facts.rs | 11 ++++++++ 4 files changed, 106 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f022adf0d..b293109d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,6 +105,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Keywords may contain `?`**: `:person/alive?` now lexes as a single keyword. Previously `?` terminated the keyword, so `[:e :alive? true]` lexed as `:alive` followed by a stray `?` symbol and was rejected as a four-element fact — reporting `Optional 4th element of a fact must be a map`, which named the value rather than the keyword. `?` is a constituent character in EDN keywords and predicate-style names (`:artist/dead?`) are idiomatic. Query variables are unaffected: a `?` not preceded by `:` still begins a symbol. `tests/grammar/grammar.pest` updated to match. +### Performance + +- **`BrowserDb` no longer allocates a redundant LRU page cache** (#275): `open_in_memory()`, `open()`, and `import_graph()` now pass page cache capacity `0` to `PersistentFactStorage::new` instead of `256`. The backing `BrowserBufferBackend` is already a `HashMap`-resident, fully-loaded page store, so every `PageCache` hit was a second RAM-to-RAM copy plus LRU bookkeeping for no benefit. + + This required fixing `PageCache` itself: capacity `0` previously meant *unbounded* (the eviction loop's `capacity > 0` guard skipped eviction but insertion still ran unconditionally), which would have made this change strictly worse — an ever-growing duplicate of every page ever read, never reclaimed. `PageCache::new(0)` now means the cache is disabled outright: `get_or_load` always reads through to the backend and `put_dirty` is a no-op. No existing caller passed `0` before this change, so no other behaviour is affected. + ## v1.2.3 — 2026-08-10 No changes to the core crate. Released so the language bindings have a version diff --git a/src/browser/mod.rs b/src/browser/mod.rs index d6e07a5b9..75daf39f0 100644 --- a/src/browser/mod.rs +++ b/src/browser/mod.rs @@ -51,8 +51,11 @@ impl BrowserDb { #[wasm_bindgen(js_name = openInMemory)] pub fn open_in_memory() -> Result { let buffer = BrowserBufferBackend::new(); - let pfs = PersistentFactStorage::new(buffer, 256) - .map_err(|e| JsValue::from_str(&e.to_string()))?; + // Page cache capacity 0: `BrowserBufferBackend` is already an in-memory + // page store, so the LRU cache would only duplicate pages and add + // HashMap overhead for zero benefit. See #275. + let pfs = + PersistentFactStorage::new(buffer, 0).map_err(|e| JsValue::from_str(&e.to_string()))?; let fact_storage = pfs.storage().clone(); Ok(BrowserDb { @@ -76,8 +79,9 @@ impl BrowserDb { let existing = idb.load_all_pages().await?; let buffer = BrowserBufferBackend::load_pages(existing); - let pfs = PersistentFactStorage::new(buffer, 256) - .map_err(|e| JsValue::from_str(&e.to_string()))?; + // Page cache capacity 0 — see comment in `open_in_memory` above. + let pfs = + PersistentFactStorage::new(buffer, 0).map_err(|e| JsValue::from_str(&e.to_string()))?; let fact_storage = pfs.storage().clone(); Ok(BrowserDb { @@ -215,7 +219,8 @@ impl BrowserDb { let (dirty_pages, has_idb) = { let mut inner = self.inner.borrow_mut(); let buffer = BrowserBufferBackend::load_pages_all_dirty(pages); - let mut new_pfs = PersistentFactStorage::new(buffer, 256) + // Page cache capacity 0 — see comment in `open_in_memory` above. + let mut new_pfs = PersistentFactStorage::new(buffer, 0) .map_err(|e| JsValue::from_str(&e.to_string()))?; let new_fact_storage = new_pfs.storage().clone(); @@ -480,6 +485,36 @@ mod tests { assert_eq!(results2[0][0], serde_json::Value::Number(30.into())); } + /// #275: `BrowserBufferBackend` is already an in-memory page store, so the + /// `PersistentFactStorage` LRU page cache is redundant on top of it and + /// should be disabled (capacity 0) for all three `BrowserDb` constructors. + #[wasm_bindgen_test] + async fn open_in_memory_disables_page_cache() { + let db = BrowserDb::open_in_memory().expect("open_in_memory"); + assert_eq!(db.inner.borrow().pfs.page_cache_capacity(), 0); + } + + #[wasm_bindgen_test] + async fn open_disables_page_cache() { + let db = BrowserDb::open("minigraf-test-zero-page-cache-open") + .await + .expect("open"); + assert_eq!(db.inner.borrow().pfs.page_cache_capacity(), 0); + } + + #[wasm_bindgen_test] + async fn import_graph_disables_page_cache() { + let db = BrowserDb::open_in_memory().expect("open_in_memory"); + db.execute(r#"(transact [[:e :v 1]])"#.to_string()) + .await + .expect("transact"); + let blob = db.export_graph().expect("export"); + + let db2 = BrowserDb::open_in_memory().expect("open_in_memory 2"); + db2.import_graph(blob).await.expect("import"); + assert_eq!(db2.inner.borrow().pfs.page_cache_capacity(), 0); + } + #[wasm_bindgen_test] async fn idb_persistence_round_trip() { let db_name = "minigraf-test-persistence"; diff --git a/src/storage/cache.rs b/src/storage/cache.rs index e95a7c077..532017a33 100644 --- a/src/storage/cache.rs +++ b/src/storage/cache.rs @@ -62,6 +62,12 @@ impl PageCache { /// Create a new page cache with the given page capacity. /// /// `capacity = 256` means at most 256 × 4KB = 1MB of cached pages. + /// + /// `capacity = 0` **disables the cache entirely**: `get_or_load` always + /// reads through to the backend and `put_dirty` is a no-op. Use this when + /// the backend itself is already an in-memory page store (e.g. a + /// `HashMap`-backed buffer), so this LRU layer would only add duplicate + /// storage and bookkeeping overhead on top of an already-resident page. pub fn new(capacity: usize) -> Self { PageCache { inner: RwLock::new(CacheInner { @@ -82,6 +88,10 @@ impl PageCache { .inner .read() .map_err(|_| anyhow::anyhow!("cache lock poisoned"))?; + // Capacity 0 disables the cache: always read through, no bookkeeping. + if inner.capacity == 0 { + return Ok(Arc::new(backend.read_page(page_id)?)); + } if let Some(entry) = inner.entries.get(&page_id) { return Ok(entry.data.clone()); } @@ -118,6 +128,10 @@ impl PageCache { /// Insert or update a page in the cache and mark it dirty. pub fn put_dirty(&self, page_id: u64, data: Vec) { let mut inner = self.inner.write().unwrap_or_else(|e| e.into_inner()); + // Capacity 0 disables the cache: nothing to prime, next get_or_load reads through. + if inner.capacity == 0 { + return; + } let data = Arc::new(data); if inner.entries.contains_key(&page_id) { // Update in place, move to MRU @@ -185,6 +199,16 @@ impl PageCache { .entries .len() } + + /// Configured capacity (in pages). `0` means the cache is disabled — see + /// [`PageCache::new`]. + #[allow(dead_code)] + pub fn capacity(&self) -> usize { + self.inner + .read() + .unwrap_or_else(|e| e.into_inner()) + .capacity + } } #[cfg(test)] @@ -286,6 +310,31 @@ mod tests { assert!(cache.cached_page_count() == 3); } + #[test] + fn test_zero_capacity_disables_caching() { + let mut backend = MemoryBackend::new(); + for i in 1u64..=10 { + backend.write_page(i, &make_page(i as u8)).unwrap(); + } + let cache = PageCache::new(0); + assert_eq!(cache.capacity(), 0); + for i in 1u64..=10 { + let page = cache.get_or_load(i, &backend).unwrap(); + assert_eq!(page[0], i as u8); + } + // Nothing should have been retained: capacity 0 means "no cache", not + // "unbounded cache" — every load must read straight through and leave + // no trace behind. + assert_eq!(cache.cached_page_count(), 0); + } + + #[test] + fn test_zero_capacity_put_dirty_is_noop() { + let cache = PageCache::new(0); + cache.put_dirty(1, make_page(0xAA)); + assert_eq!(cache.cached_page_count(), 0); + } + /// Regression test for: put_dirty on a cached page after an eviction caused /// an out-of-bounds panic. The positions HashMap (now removed) became stale /// after pop_front shifted all remaining VecDeque indices by 1, so touch() diff --git a/src/storage/persistent_facts.rs b/src/storage/persistent_facts.rs index 800b0942f..7e7cef678 100644 --- a/src/storage/persistent_facts.rs +++ b/src/storage/persistent_facts.rs @@ -1052,6 +1052,17 @@ impl PersistentFactStorage { self.dirty } + /// Capacity (in pages) of the internal LRU page cache passed to `new()`. + /// + /// Exposed only for the browser WASM layer's tests, which verify that + /// `BrowserDb` constructors pass `0` (cache disabled — see #275) since + /// `BrowserBufferBackend` is already an in-memory page store and the LRU + /// layer on top of it would be redundant. + #[cfg(all(target_arch = "wasm32", feature = "browser", test))] + pub(crate) fn page_cache_capacity(&self) -> usize { + self.page_cache.capacity() + } + /// Run a closure with read access to the underlying storage backend. /// /// Used by the browser WASM layer to read pages after `save()` without