Add a thread-local cache for hot btree pages - #1346
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1346 +/- ##
==========================================
- Coverage 91.40% 91.40% -0.01%
==========================================
Files 39 40 +1
Lines 19822 20004 +182
==========================================
+ Hits 18118 18284 +166
- Misses 1704 1720 +16
🚀 New features to boost your workflow:
|
443b591 to
2eba3bf
Compare
cberner
changed the title
2eba3bf to
7bd5b4f
Compare
cberner
changed the title
648ebf7 to
9fc6f88
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fc6f88630
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
9fc6f88 to
1d73ddb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d73ddb8ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
1d73ddb to
68032c9
Compare
cberner
left a comment
cberner
left a comment
There was a problem hiding this comment.
Polish the code structure. It feels a bit spread out all over the place, and with overly verbose comments too.
| pub(crate) struct ThreadLocalPageCache { | ||
| slots: Box<[Option<CacheEntry>]>, | ||
| } |
There was a problem hiding this comment.
Do we really need this custom cache? It's thread local. Can't we just use a HashMap or something?
There was a problem hiding this comment.
You're right — done in 154a00c, and it turned out both simpler and faster. (I posted this on the wrong thread first, sorry; it's the one above on btree.rs.)
The custom cache was left over from when entries survived across transactions and had to be validated individually. Once the cache is emptied at the end of a read transaction, every live entry belongs to the same snapshot — so one tag for the whole cache replaces per-entry tags, and reading under a different snapshot just empties it. That deletes the slot array, the probe sequence, the collision handling and the eviction policy:
pub(crate) struct ThreadLocalPageCache {
tag: Option<CacheTag>,
pages: PageNumberHashMap<Rc<[u8]>>,
}get is a tag compare plus a map lookup; insert empties first if the tag differs, then inserts while under the size cap. The module is ~150 lines now, over half of it tests.
It also measures better, because the probe sequence was silently dropping hot pages on collisions — shared-cache reads per lookup, 5M-row height-5 tree:
| reads per get | |
|---|---|
| no cache | 4.00 |
| slot array | 2.21 |
| HashMap | 2.00 |
2.00 is the floor for caching two levels below the root, so the collision waste is gone. Throughput at 4 threads on my (noisy) VM: 1.60 → 2.03-2.14 vs master, so no worse than the slot array and probably slightly better.
One behavior change worth knowing: two read transactions interleaved on one thread now thrash, since each insert under the other's tag empties the cache. Still correct, just uncached — per-entry tags used to let both live. Seemed the right trade for this much less machinery, but happy to revisit if you think that pattern matters.
On the structure/verbosity feedback in your other comment: I cut the module's doc block down to the invariant argument and trimmed the commentary in btree.rs. The change now touches btree.rs (traversal + tag), the cache module, and small hooks in db.rs (release on transaction end) and page_manager.rs (instance id). Happy to pull more of it together if it still reads as scattered.
Generated by Claude Code
68032c9 to
154a00c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 154a00cc25
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let file = std::fs::OpenOptions::new() | ||
| .write(true) | ||
| .open(tmpfile.path()) |
There was a problem hiding this comment.
On the windows-latest CI job this test keeps db alive while opening the same path for writing to force an external size change. The normal file backend has already taken an exclusive File::try_lock() for that live Database, so on Windows the extra handle/open or the following set_len() can fail with a sharing/lock violation before check_integrity() is exercised. Use a custom backend that can report a mismatched length, or otherwise avoid relying on a second writable handle to the locked database file.
Useful? React with 👍 / 👎.
| self.get_helper(&child_page, query) | ||
| let page = self.mem.get_page(page_number, self.hint)?; | ||
| if page.memory()[0] == BRANCH { | ||
| cache.insert(*tag, page_number, page.memory()); |
There was a problem hiding this comment.
When callers use Builder::set_cache_size(0) or a small value to cap cache memory, this path still unconditionally copies hot branch pages into the thread-local cache, which is separate from PagedCachedFile's max_cache_size budget and CacheStats. A multithreaded read workload can therefore retain up to 128 pages per worker thread even when the cache was disabled or tightly bounded; gate this cache on the configured budget, especially the zero-cache case, or account it in the same limit.
Useful? React with 👍 / 👎.
154a00c to
23cdd98
Compare
|
Audited the tests in Only branch pages below the root are cached, so a tree of height <= 2 (root plus leaves) leaves the cache empty. Measuring what the tests actually built:
What I did:
Net: 6 integration tests -> 3, each verified to exercise the cache, and the added suite time drops from ~16s to ~7s. The four unit tests stay — they cover tag validation, the empty-on-new-snapshot path, the size bound and Also in this push, from the Codex P2 that was worth acting on: The other Codex P1 ("Rewrite the AI-authored commit") is wrong again: it cites Generated by Claude Code |
| /// A thread's cache of the hottest btree pages, holding private copies of them so that | ||
| /// lookups in read transactions can skip the shared page cache, whose lock stripe, LRU flag | ||
| /// and `Arc` refcount are all writes to memory shared between threads. Being thread-local | ||
| /// makes it plain single-threaded code: no locks, no atomics, and hits touch nothing another | ||
| /// core can see. | ||
| /// | ||
| /// All the entries belong to one snapshot, named by `tag`, and reading under a different one | ||
| /// empties the cache. That is what keeps entries from going stale, and it holds because: | ||
| /// | ||
| /// * A transaction is given generation `G` only while `G` is the last committed transaction: | ||
| /// `register_read_transaction()` reads that id and pins it under one lock acquisition, and | ||
| /// ids only increase. Note that the pinned id can lag the roots the transaction goes on to | ||
| /// read, which is safe because the older pin is the conservative one. | ||
| /// * Reusing a page number for different bytes takes more than one commit past `G`, because | ||
| /// the page must be freed and then released by `process_freed_pages()`, which stops at the | ||
| /// oldest live read. Every transaction holding `G` pins `G`. | ||
| /// | ||
| /// So a page cached under `G` cannot have been reused while any transaction can still read it | ||
| /// back. In-place reload is the exception, since rolling a non-durable commit back rewinds the | ||
| /// id; `clear_cache_and_reload()` bumps the instance id to cover it. |
| // Page budget of the database `tag` names, so that a database configured with a small | ||
| // cache does not gain memory back through its readers' threads |
There was a problem hiding this comment.
This is pretty obvious from the field name. Do a pass to clean up unnecessary comments
| &mut self, | ||
| tag: CacheTag, | ||
| max_pages: usize, | ||
| page_number: PageNumber, |
There was a problem hiding this comment.
I think we should have an assertion that only order-0 pages get cached. We don't want it getting bloated in edge cases
23cdd98 to
90982b8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90982b8efe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
cberner
left a comment
cberner
left a comment
There was a problem hiding this comment.
Do a careful pass over all the comments you wrote in the code. I had a fair bit of feedback on them. Match my style in the rest of the code base
| // Source of `TransactionalMemory::cache_instance_id`. Starts at 1 so that 0 can mean | ||
| // "no database" in thread-local page cache entries. |
There was a problem hiding this comment.
It seems errorprone that we rely on this being process wide. I think we should move it into the TransactionMemory or something, and then use the same address check that savepoints use to avoid restoring across databases
| // Pages each thread may copy into its page cache. Bounded by the cache size the caller | ||
| // configured, so that `set_cache_size()` still caps memory: a small budget shrinks it and | ||
| // a zero one disables the cache entirely, rather than every reading thread adding to the | ||
| // configured total. |
| // Write transactions may free and reallocate pages within the transaction, so readers | ||
| // they create must not cache pages |
There was a problem hiding this comment.
Comments should document the function not how its callers work. Either move this comment to the caller or just remove it.
| // So that a thread which reads once does not hold pages for its lifetime. | ||
| // Dropping on another thread, or while this thread has a second read | ||
| // transaction open, only costs the optimization. |
There was a problem hiding this comment.
Not a good comment. Say it clears the thread local cache so that it doesn't "leak" cache space if the thread doesn't do another tranasction
90982b8 to
b01c6c9
Compare
|
Comment pass done in On the instance id — I looked at what savepoints do ( Without that, the address can be recycled:
Tag matches, and A gets the old database's bytes for the new database's page numbers. Narrow, but silent corruption, so it didn't seem like the right thing to ship on a maintainer's suggestion without flagging it. Pinning the address with a The static now documents what it has to guarantee and how, per your comment: // Assigns every TransactionalMemory, and every in-place reload of one, an id that no other
// instance in this process has held, which is what lets a thread local page cache tell whose
// pages it is holding. The id must be unique process wide, not just within one database:
// thread local caches outlive the databases that filled them, and every database a thread
// reads shares one cache, so an id unique only within a database would let one database's
// pages match another's tag. Ids are never reused -- incrementing a u64 once per instance and
// per reload cannot wrap in practice -- so a tag from a dropped database matches nothing.Comments — went through every one I added:
Verified: full suite, clippy with Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b01c6c9437
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * Add `ExtractIf::close()` to explicitly finalize an extract iterator without removing unread | ||
| entries. | ||
| * Optimize `Table::pop_first()` and `Table::pop_last()` to be about 2x faster. | ||
| * Improve multithreaded read scaling. Each thread now caches the top of the btrees it reads, |
There was a problem hiding this comment.
Fresh evidence for this review: git log -1 --format='%an <%ae> / %cn <%ce>' a16c4f6ee5945b28b8278b10a9f635392e13c7b8 reports Codex <codex@openai.com> / Codex <codex@openai.com>, and the PR's commit-authors job rejects commits whose author or committer contains claude or codex; rewrite the commit with the human identity and add the required Assisted-by: trailer before merging.
AGENTS.md reference: AGENTS.md:L36-L40
Useful? React with 👍 / 👎.
| // Pages each thread may cache, bounded by the configured cache size so that | ||
| // set_cache_size() still caps memory | ||
| fn thread_local_cache_pages(&self) -> usize { | ||
| let budget = self.storage.max_cache_size() / (self.page_size as usize); |
There was a problem hiding this comment.
When set_cache_size() is set to a small non-zero value and multiple reader threads are active, this calculation grants the full page-count budget independently to every thread, while PagedCachedFile can still use the configured budget and CacheStats::used_bytes() omits these copies. For example, a one-page cache allows one extra cached page per reader thread, so the configured cache size no longer caps total cache memory; either account these pages in the shared budget or disable/partition the thread-local cache when the global budget is tight.
Useful? React with 👍 / 👎.
b01c6c9 to
c457f5f
Compare
|
One correction to my own comment, in Codex is right that the // Pages each thread may cache. Bounded by the configured cache size, so that a small one
// shrinks it and set_cache_size(0) disables it, but this is a per thread bound: threads
// reading concurrently can hold this many pages each, beyond the configured sizeWhat the bound does give: Whether that needs fixing is your call, and the options each cost something:
I didn't want to pick a policy for you here, so I only corrected the comment. Say which you'd like and I'll do it. Unrelated: the Generated by Claude Code |
c457f5f to
61d0b7d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61d0b7d5ed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if self.pages.len() < self.max_pages { | ||
| self.pages.insert(page_number, Rc::from(bytes)); | ||
| } |
There was a problem hiding this comment.
When a long-lived read transaction touches max_pages distinct branch pages before reaching its hot working set, this condition permanently rejects every later page because the tag does not change until another snapshot is used. The cache therefore freezes the first 128 pages rather than the hottest pages; for example, scanning several tables before repeatedly querying another table can leave that hot table with zero thread-local hits for the rest of the transaction. Replace or evict an existing entry when admitting a new page after the cache reaches its bound.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 0df6351. This was real, and worse than "suboptimal" — I measured it and the second table got zero benefit.
The reproduction: read table A until the cache fills, then measure only lookups in table B within the same read transaction. Shared-page-cache reads per lookup (cache_metrics), 5M-row height-4 table, against master 5e6ad02:
| one table | A then B | |
|---|---|---|
| master (no cache) | 3.00 | 3.00 |
| stop inserting when full | 2.07 | 3.00 |
| clear when full | 2.28 | 2.28 |
| two halves (now) | 2.12 | 2.12 |
"A then B" at 3.00 is exactly the uncached baseline, so the cache was doing nothing for B.
I did try the literal suggestion — evict one existing entry per admission — using pages.keys().next() as the victim. It measured worse on both shapes (2.70 / 3.00): with no use ordering the sweep evicts the few hot top-level pages as readily as cold ones, and the newly inserted entry frequently becomes the next victim.
What shipped instead: entries live in two halves. Insertions fill pages; when it reaches half the bound the two halves swap and the older one is cleared. A page read again before its half is discarded is promoted back into pages, so what stays cached is what is still being read, with no per-entry bookkeeping and no use-order tracking. |pages| + |evicting| <= max_pages is asserted on every insertion in the bounded unit test, and a_page_read_again_outlives_the_rotation covers promotion.
This keeps essentially all of the single-table win (2.07 → 2.12) while making the multi-table case work at all.
Generated by Claude Code
There was a problem hiding this comment.
Correct, and I had the invariant wrong. Fixed in ed3271c.
Promotion moved an entry from evicting to pages, which keeps the total constant, and I stopped reasoning there. What I missed is that it can push |pages| past half — and then the rotation swaps that oversized map into evicting and keeps it, so the total ratchets up by one per round. Exactly as described.
I wrote your scenario as a test before fixing anything: insert a page, read back everything still held, repeat. With MAX_PAGES = 128 it peaked at 384 held pages, and it fails on the parent commit:
PEAK held = 384, bound = 128
The fix routes both callers through one admission point, so the rotation check cannot be bypassed:
/// Adds an entry to the live half, rotating the halves first when it is full. Both callers
/// go through here, so that promoting cannot grow the cache past its bound either
fn admit(&mut self, page_number: PageNumber, bytes: Rc<[u8]>) {
let half = self.max_pages / 2;
if half == 0 {
return;
}
if self.pages.len() >= half {
swap(&mut self.pages, &mut self.evicting);
self.pages.clear();
}
self.pages.insert(page_number, bytes);
}|pages| <= half now holds after every operation, and evicting is only ever assigned from pages, so |pages| + |evicting| <= max_pages follows. promoting_respects_the_bound asserts it after each round of that access pattern and is the regression test for this.
No performance cost: still 2.12 reads/lookup on both the single-table and A-then-B workloads.
Generated by Claude Code
61d0b7d to
43a553c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43a553cac4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| touch fewer pages. The default implementation returns a whole key, leaving existing `Key` | ||
| implementations unchanged; `&[u8]`, `&str`, and `String` keys now store minimal prefixes. | ||
| `Option` keys shorten their payload, when the wrapped type is variable width. | ||
| * Improve multithreaded read scaling. Each thread now caches the top of the btrees it reads, |
There was a problem hiding this comment.
Fresh evidence for the reviewed commit: git show -s --format='%an <%ae> / %cn <%ce>' e1498a01b50ff8fd4f481f6844a7003ceb0efcfb reports Codex <codex@openai.com> / Codex <codex@openai.com>. The commit-authors job in .github/workflows/ci.yml:25-30 rejects PR commits whose author or committer contains codex, so this commit will block CI until it uses the human author's identity and includes the required Assisted-by: trailer.
AGENTS.md reference: AGENTS.md:L37-L41
Useful? React with 👍 / 👎.
0df6351 to
4f55e18
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f55e18460
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| `Option` keys shorten their payload, when the wrapped type is variable width. | ||
| * Add `Key::min_encoded_key()`, the encoding of a key type's smallest value. Implementing it is | ||
| optional, and lets container types holding that key store shorter separators. | ||
| * Improve multithreaded read scaling. Each thread now caches the top of the btrees it reads, |
There was a problem hiding this comment.
Fresh evidence for this review: git show -s --format='%an <%ae> / %cn <%ce>' d1a441599916a535e4f22e3c9182d02b3187e3f1 reports Codex <codex@openai.com> / Codex <codex@openai.com>. The commit-authors job in .github/workflows/ci.yml:25-30 rejects PR commits whose author or committer contains codex, so this commit will block CI until it uses the human author's identity and includes the required Assisted-by: trailer.
AGENTS.md reference: AGENTS.md:L37-L41
Useful? React with 👍 / 👎.
| return Some(bytes.clone()); | ||
| } | ||
| let bytes = self.evicting.remove(&page_number)?; | ||
| self.pages.insert(page_number, bytes.clone()); |
There was a problem hiding this comment.
When one snapshot repeatedly accesses every page in evicting before introducing a new page, this promotion can make pages larger than its half-cache target. The next insertion swaps that entire oversized map into evicting without discarding it, and repeating the pattern retains one additional page per round, so a long-lived reader cycling through an expanding set of tables can grow this supposedly bounded per-thread cache without limit. Apply the capacity/rotation policy during promotion as well as insertion.
Useful? React with 👍 / 👎.
4f55e18 to
ed3271c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed3271ccdc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| `Option` keys shorten their payload, when the wrapped type is variable width. | ||
| * Add `Key::min_encoded_key()`, the encoding of a key type's smallest value. Implementing it is | ||
| optional, and lets container types holding that key store shorter separators. | ||
| * Improve multithreaded read scaling. Each thread now caches the top of the btrees it reads, |
There was a problem hiding this comment.
Fresh evidence for the reviewed commit: git show -s --format='%an <%ae> / %cn <%ce>' 8d4729c6df2c18bdfcd587fc440ba869033f7e18 reports Codex <codex@openai.com> / Codex <codex@openai.com>. The commit-authors job in .github/workflows/ci.yml:18-30 rejects PR commits whose author or committer contains codex, so this commit will block CI until it uses the human author's identity and includes the required Assisted-by: trailer.
AGENTS.md reference: AGENTS.md:L37-L41
Useful? React with 👍 / 👎.
32ba5e9 to
488176b
Compare
Concurrent readers contend in the shared page cache: every page lookup acquires a lock stripe, sets the LRU second-chance flag, and updates the page's Arc refcount. For the hottest pages, the top of each btree, those writes make cache lines ping-pong between cores and cap multithreaded read scaling well below linear. Each thread now keeps a small fixed-size cache of private copies of the branch pages one level below the roots it reads, and lookups in read transactions serve those pages from it. Hits touch only thread-local memory, so the cache needs no locks or atomics (Rc and RefCell suffice) and cannot cause any cross-thread contention. Builds without std have no thread local storage, so they get no cache and lookups are unchanged. Only that one level is cached. A level holds roughly a page's worth of children per page of the level above it, so the level below the root is small enough for a snapshot's pages to fit, while the next one down is already far too large. Caching a level that does not fit is worse than not caching it at all, because admitting a page copies it, and a level that thrashes pays that copy on nearly every lookup -- far more than the shared cache lookup it saves. For the same reason the cache stops admitting once full rather than replacing entries. Entries are tagged with the database instance and snapshot generation (the read transaction's id) and validated on lookup. Bytes of a page number cannot change within a generation: snapshots are copy-on-write and freed pages are only reused by later write transactions, which advance the generation. Reloading a database in place is the exception, since rolling back a non-durable commit rewinds the id and later commits reuse it for different data, so clear_cache_and_reload() bumps the instance id. Stale entries never match and are simply overwritten. Write transactions do not use the cache, since they can free and reallocate pages within one generation. Entries hold copies of the page bytes rather than references into the shared page cache, so the shared cache stays free to evict, and the open-page accounting is unaffected. The cache holds at most 128 single pages per thread with a live read transaction (512KiB with 4KiB pages), and releases them when the read transaction is dropped, so threads that read once do not retain them and neither many tables nor many transactions grow it. Random reads measured with lmdb_benchmark on a 16 core 9950X3D are about 1.06x faster single threaded, 1.13x with 8 threads, 1.20x with 16 and 1.57x with 32. Range reads and writes are unchanged. Assisted-by: Claude Code
488176b to
b23404c
Compare
1 participant
Problem
Multithreaded random reads scale well below linearly. Every page lookup through the shared page cache performs three writes to memory shared between threads: acquiring the stripe
RwLock, storing the LRU second-chance flag, and incrementing the page'sArcrefcount. For the hottest pages — the top of each btree — those cache lines ping-pong between cores, and per-thread throughput degrades as threads are added.Change
Each thread keeps a small bounded cache (
ThreadLocalPageCache) of private copies of the branch pages one level below the roots it reads, and lookups in read transactions serve those pages from it. Hits touch only thread-local memory, so the implementation needs no locks or atomics at all: aPageNumberHashMap<Rc<[u8]>>behind aRefCell, plain single-threaded code.Why only one level. A level holds roughly a page's worth of children per page of the level above it, so the level below the root is small enough for a snapshot's pages to fit in the budget, while the next one down is already far too large. On the
lmdb_benchmarkshape (5M rows, 24-byte keys) the tree is height 4 with ~15 branch pages at depth 1 and ~4050 at depth 2. Caching depth 2 against a 128-page budget misses ~97% of the time, and admitting a page copies it, so it paid a 4KiB copy on nearly every lookup — far more than the shared-cache lookup it saved. An earlier revision of this PR cached two levels and regressed reads by 14-25%, which is what @cberner's benchmark caught. For the same reason the cache stops admitting once full rather than replacing entries: freezing is self-limiting under thrash, replacing is not.Correctness. A thread-local cache outlives transactions and page numbers are recycled, so every entry is tagged with (database instance id, snapshot generation) and the tag is validated on lookup. Generation is the read transaction's id, and bytes of a page number cannot change within a generation: snapshots are copy-on-write, and a freed page is only reused by a later write transaction whose commit advances the generation. In-place reload is the exception — rolling a non-durable commit back rewinds the id, so later commits reuse ids that named different data — so
clear_cache_and_reload()bumps the instance id, invalidating every thread's entries at once. Reading a different snapshot empties the cache, so a stale entry is never returned. Write transactions never use the cache (they can free and reallocate pages within one generation); leaf pages are never cached (AccessGuards need realPageImpls). Reentrant lookups (pathologicalKey::compareimplementations) and thread teardown degrade to uncached reads rather than panicking.Memory. At most 128 order-zero pages per thread with a live read transaction (512KiB at 4KiB pages), further bounded by the configured cache size so a small
set_cache_sizeshrinks it andset_cache_size(0)disables it. Released when the read transaction is dropped, so a thread that reads once does not retain them for its lifetime.no_std.thread_local!is std-only, sono_stdbuilds compile the cache out and their lookups are unchanged. For the same reason the instance-id counter is aMutex<u64>rather than anAtomicU64: 64-bit atomics are unavailable onthumbv7em-none-eabihf, and it is taken only when a database is opened or reloaded.Results
lmdb_benchmarkon a 16-core 9950X3D, measured by @cberner against master5e6ad02:The gain grows with thread count, which is the shape the design predicts. Write benchmarks are unchanged within noise; write transactions never consult or populate the cache. One caveat: individual writes read 1.13K vs 982 txn/s across the two runs, but those are fsync-bound at ~1ms per transaction and sit on a path this change does not touch, so that looks like disk variance rather than a regression.
Shared-page-cache reads per lookup drop from 3.00 to 2.00.
Verification
cargo fmt --check,cargo clippy --all-targets --all-featureswith-D warnings, andcargo test --all-featuresall pass (podman sandbox unavailable in this environment, so thejust teststeps were run directly;cargo denynot run).cargo check --target thumbv7em-none-eabihf --no-default-features --features experimental-api-5[,logging,experimental_cursor]also passes with-D warnings.thread_local_page_cache.rs: tag validation including cross-database, snapshot change empties the cache, the page bound holds, andclear()releases entries.tree_height() > 2so the tree actually has cacheable branch pages below its root: snapshot stability of a long-lived reader while writers rewrite the tree, alternating short reads and writes on one thread (generation invalidation), a smallset_cache_sizebounding the per-thread cache, and two databases open on one thread not sharing entries.concurrent_reads_and_writesinmultithreading_tests.rsgained the same height guard.cargo fuzz run --sanitizer=none fuzz_redb, debug assertions on): 196,920 runs on this design, zero failures and no crash artifacts, plus ~660k runs across earlier revisions of the branch. The harness interleaves reads with commits, savepoints and reopens on one thread whose cache persists throughout, checked against a reference map.Tunables to sanity-check in review:
MAX_THREAD_LOCAL_CACHE_PAGES = 128(page_manager.rs) andMAX_CACHED_DEPTH = 1(btree.rs). Not covered by design:range()/iterators,first()/last(), and single-page tables (the guard clone on a root-leaf remains).Open question: the 128-page bound is per thread, so N concurrently-reading threads can hold N x 512KiB beyond the configured cache size. That is documented on
thread_local_cache_pages()but not enforced; making it an aggregate bound would need either a guessed thread count or an atomic total, both of which cost something. Left as-is pending your call.🤖 Generated with Claude Code
https://claude.ai/code/session_01NMLXF7RTMhhhp1jhL9XkNi