Repository navigation
perf: skip redundant page cache allocation in BrowserDb - #342
Merged
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMWSutMLJJ7Y5b8hs9ZaCU
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! 8000 |
adityamukho
added a commit
that referenced
this pull request
Oct 7, 2026
…the IndexedDB flush (#470) checkpoint(), importGraph() and every write collected dirty pages with read_page_raw(id).ok() in a filter_map, so a page that failed to read was silently left out of the flush to IndexedDB while the call reported success. A shared take_dirty_pages helper (the same one v3 has since #469) now returns the read error. No current path leaves a dirty id without a page (ids enter the dirty set only after the page is stored), so this is defensive rather than a fix for observed loss. The browser unit tests did not compile on wasm32 (a duplicate page_cache_capacity from #341/#342, and dir_sync tests using tempfile); both are fixed so the two new regression tests run. Claude-Session: https://claude.ai/code/session_015GgLEs8YozJjYhwPEYgTUn Co-authored-by: Claude Opus 5.5 <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
BrowserDb::open_in_memory(),open(), andimport_graph()now pass page cache capacity0toPersistentFactStorage::newinstead of256.BrowserBufferBackendalready keeps every page resident in aHashMap(pre-loaded from IndexedDB at open time), so the LRUPageCacheon top of it was a redundant RAM-to-RAM copy plus bookkeeping on every read.PageCacheuncovered while implementing this:PageCache::new(0)previously meant unbounded, not disabled — the eviction loop'scapacity > 0guard skipped eviction, but insertion still ran unconditionally, so every page ever read would have accumulated forever with no eviction. Naively wiring0through without this fix would have made memory usage strictly worse than the status quo.capacity == 0now disables the cache outright:get_or_loadreads straight through to the backend andput_dirtyis a no-op. No existing caller passed0before this change, so no other behaviour is affected (verified: full native test suite passes unchanged).PageCache::capacity()and a#[cfg(all(target_arch = "wasm32", feature = "browser", test))]-gatedPersistentFactStorage::page_cache_capacity()accessor so the newBrowserDbtests can assert the constructors wire this up correctly. The accessor is test-only and compiled out of release WASM builds.OpenOptions::page_cache_size(nativesrc/db.rs) was intentionally left untouched —BrowserDbdoesn't useOpenOptionsat all, and that option is native-only territory owned by the companion issue perf: skip page cache allocation for in-memory databases #274.Closes #275
Test plan
cargo test(native, full suite) — 0 failed, all passing, including 2 newPageCachezero-capacity unit testscargo clippy --all-features -- -D warnings— cleancargo fmt --check— cleancargo check --target wasm32-unknown-unknown --features browser --tests— compiles clean, confirming the 3 new#[wasm_bindgen_test]assertions (open_in_memory_disables_page_cache,open_disables_page_cache,import_graph_disables_page_cache) type-check against the realBrowserDb/PersistentFactStorageAPIwasm32+browsertarget (onlyRustworkflow's nativecargo build/cargo testacross OSes, plus--all-featuresclippy on native), and no headless-Chrome/wasm-pack testrunner was available in this environment, so the newwasm_bindgen_tests could not be executed end-to-end locally — only compiled. They exercise real code paths already covered indirectly by the existingin_memory_transact_and_query/idb_persistence_round_triptests in the same file, so correctness risk is low.🤖 Generated with Claude Code
https://claude.ai/code/session_01YMWSutMLJJ7Y5b8hs9ZaCU