perf: interior-mutable S3-FIFO freq bump so GET is a shared read (#576 COW prerequisite)#587
Merged
Merged
Conversation
… read (#576 prereq) The GET read path used find_mut + Entry::bump_freq, taking a unique &mut on the slot table and the entry just to bump the S3-FIFO 2-bit promote frequency. That &mut is the measured driver of the per-slot Arc-COW regression: with the value behind an Arc, a freq bump on GET forces Arc::get_mut's atomic strong/weak refcount check on the hot path. A de-risk microbench put that at roughly 11% on GET; making the freq bump a SHARED read drops it under the gate to roughly 1 to 2%. This PR makes only the freq-bump decoupling, with no Arc and no COW yet, so it is small, independently reviewable, and independently measured. The change adds Entry::bump_freq_shared(&self), an interior-mutable freq bump, and switches the read (GET) and read-only rmw (e.g. GETEX) paths from slot_table_mut().find_mut().bump_freq() to slot_table().find().bump_freq_shared(). The whole hit path is now a shared borrow of the table and the entry; the mutating bump_freq(&mut self) stays for the paths that already hold &mut (rmw_mut, eviction second-chance, dec). Soundness (single-threaded per shard, ADR-0002/0005): Entry is an 8-byte tagged NonNull<u8> that solely owns its pointee (a Str blob or a Box<CollEntry>). The freq lives INSIDE that pointee, a separate allocation from the Entry's own pointer word, so the &self borrow, which covers only the pointer word, does not forbid mutating the pointee. bump_freq_shared loads the owned raw pointer (raw pointers are Copy and carry their allocation's write provenance) and patches a single byte through it, forming no overlapping shared/mutable view of that byte for the write. There is no cross-thread aliasing: a shard's Entrys are owned by exactly one core and never sent across threads (the store is !Send/!Sync), so a plain non-atomic write is the correct primitive; an atomic would be pure overhead. The freq is a bounded 0..=3 counter, not a clock or entropy source, so this stays deterministic (ADR-0003). Eviction is BYTE-UNCHANGED: bump_freq_shared computes the same (freq + 1).min(3) and writes the same bits as bump_freq, for both the Str (flags bits 1-2) and Coll (eviction_rank) arms. The full ironcache-store + ironcache-server + ironcache suites pass unchanged, including every eviction integration test (evict_to_fit determinism on replay, lowest-freq pool eviction, hot-key survival, pool carryover determinism), plus two new unit tests asserting the shared bump is byte-identical to the mutable bump through a &Entry across a full access sweep. Measured GET delta (criterion store/read_hit_embstr, 10k embstr keys, mid-table hot key, Docker rust:1-bookworm): median 27.382 ns before, 27.453 ns after; change [+0.21% +0.70% +1.23%], which criterion reports as within the noise threshold. Neutral as expected, since this only changes HOW the freq is bumped (a raw-pointer byte write vs a &mut slice write), adding no indirection. Part of #576 (per-slot Arc-COW prerequisite). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Zeke <ezequiel.lares@outlook.com>
perf-gate (A5)Same-runner ratchet of HEAD against the merge-base (both rebuilt and measured in this job).
Overall: PASS
|
ELares
added a commit
that referenced
this pull request
Jul 9, 2026
… frozen shard off-thread (#576) (#588) The concurrent-snapshot p99.9 catastrophe (3.5s vs a ~40ms baseline on c7g) was NOT the encode+fsync (#586 already moved those off-thread) but the O(N) `snapshot_chunk` COPY of the whole keyspace into owned records, which HAD to stay on the serving core (the store is `!Send`) and contended the datapath on memory bandwidth. #571 (yield) only interleaved it and #578 (throttle) only STRETCHED it over a longer window, both measured to help nothing / hurt. This removes the serving-side copy entirely via per-slot Arc copy-on-write: - STORE: `dbs` is now `Vec<Vec<Arc<HashTable<Entry>>>>`. GET reads through the Arc as a SHARED deref (`slot_table`) -- one pointer indirection, NO `Arc::get_mut`, NO atomic on the read path (keeps GET at +1-2% with #587's shared freq bump). A write takes `Arc::make_mut` (`slot_table_mut`): the common not-saving case is strong_count==1, an uncontended in-place mutate; while a save holds a frozen clone (strong_count>1) the first write to a slot deep-clones it. - DEEP-CLONE-ON-COW (the correctness crux): `Arc::make_mut` on a frozen slot clones `HashTable<Entry>`, which clones each `Entry`, and `Entry`'s `Clone` is a DEEP clone (a fresh pointee allocation -- Str blob copied, CollEntry re-boxed). The fresh table becomes the live slot; the frozen Arc retains the ORIGINAL entries, sole-owned by the persist thread. So a write during a save is NEVER visible in the dump and NEVER mutates/frees a pointee the persist thread reads: no data race, no use-after-free, no double-free. A shallow (pointer-copy) clone would be catastrophic here; `Entry` is verified NOT `Clone`-shallow. - FrozenSlot unsafe Send: `Arc<HashTable<Entry>>` is `!Send` (Entry is a `!Send` tagged NonNull). `struct FrozenSlot(Arc<..>)` with `unsafe impl Send` hands one slot to the persist thread. SOUND because a frozen slot is de-facto IMMUTABLE for the save: every datapath write COWs first (above), and the one shared-`&Entry` write -- the S3-FIFO freq bump (`bump_freq_shared` on GET/rmw) -- is GATED OFF via a per-shard `saving` flag for the save window, so no byte the persist thread reads is mutated. The Arc refcount is atomic, so the cross-thread move/drop is race-free; dropping a FrozenSlot frees pointees only when strong_count hit 1 (the slot was COW'd away, no live reader remains). This mirrors why the store is otherwise !Send (mutable aliasing on the owning core), which a frozen slot lacks. - SAVING FLAG fast path: a plain `bool` (single-threaded shard, ADR-0002/0005; the persist thread never touches it). `begin_save` Arc-clones every non-empty slot (O(slots), no key copy) and sets the flag; `end_save` clears it. The datapath fast path (not saving) is one predictable branch + the Arc deref. - SAVE PATH (coordinator `save_shard_local`): FREEZE via `begin_save`, hand the `Vec<FrozenSlot>` to the dedicated `ic-persist-<n>` thread (#586 pattern), which runs the whole O(N) encode+fsync (`dump_frozen_slots` -> `to_kvobj` + encode + CRC + `write_shard_dump`) OFF the serving core, drops the frozen Arcs, and signals a oneshot. The shard AWAITS that (a cross-thread wake, not a blocking join) and serves throughout. `end_save` runs on the normal-completion path only (all arms mean the persist thread's closure has fully exited); on task cancellation the flag safely stays set on an exiting process (never racing the still-running thread). - CRASH-SAFETY preserved (#530): the persist thread writes only the per-shard file atomically; the manifest is still written LAST on the home core. A panicked persist thread -> a failed save (record_save_failed), never a hang; frozen Arcs dropped on unwind. - CONSISTENCY: the dump is now a per-shard POINT-IN-TIME as of the freeze (STRONGER than the #571 chunked walk -- a mid-save write keeps its pre-freeze value in the dump, or is omitted if newly created). Cross-shard stays fuzzy (each shard freezes at its own instant), the accepted cache warm-start tradeoff. The #578 throttle knob stays settable but is inert for the tail (the copy it throttled is gone). TESTS (this is the correctness-critical PR): - store `cow_deep_clone_isolates_frozen_slots_from_overwrite_and_delete`: overwrite + delete every frozen key on the LIVE store while holding the FrozenSlots; the frozen view keeps its pre-freeze values byte-for-byte, the live store advances, and drop ordering is UAF/double-free-free. - store `freq_bump_skipped_while_saving_then_resumes`: the soundness gate. - store `point_in_time_holds_under_mass_writes_at_scale` (200k keys). - persist `dump_frozen_slots_is_point_in_time_across_later_writes`: dump + RELOAD shows the frozen point-in-time; post-freeze overwrites/deletes never corrupt it. - integration `bgsave_cow_serves_inplace_writes_and_reloads_consistently`: real APPEND/INCR/DEL/SET during a BGSAVE, each serviced with low latency, reload self-consistent, live writes present. - MIRI: `cargo +nightly miri test` ran BOTH focused store tests clean (no Undefined Behavior / UAF / double-free), including the deep-clone-on-COW + FrozenSlot drop ordering crux (91s). - clippy -D warnings clean, fmt clean, CI invariants + prior-art pass, io_uring build ok; existing bgsave/off-thread/backpressure/crash/torn/turnkey_restart tests all pass. Part of #576. Claude-Session: https://claude.ai/code/session_01VFWCZR47TDvsFPJ1waX85X Signed-off-by: Zeke <ezequiel.lares@outlook.com> Co-authored-by: Claude Fable 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
1 participant
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.
What
The prerequisite for the #576 per-slot Arc-COW snapshot fix. A de-risk microbench proved Arc-COW regresses GET ~11% ONLY because IronCache's GET bumps the S3-FIFO frequency via
find_mut-- which, with the table behind anArc, would forceArc::get_mut's atomic refcount check on every GET. Making the freq bump INTERIOR-MUTABLE (so a GET hit is a SHARED read) drops that to ~1-2% (under the gate). This does that change ON ITS OWN, small + independently measured.Entry::bump_freq_shared(&self)read-modify-writes the freq through the entry's own raw pointer (Str: the flags byte atSTR_BLOB_OFFSET + FLAGS_OFFSET; Coll:CollEntry.eviction_rank). The GETread+ read-onlyrmwpaths went fromslot_table_mut().find_mut().bump_freq()toslot_table().find().bump_freq_shared(); the mutatingbump_freq(&mut self)stays for paths that already hold&mut(eviction second-chance,dec,rmw_mut).Safety (documented in the accessor): the freq lives INSIDE the pointee, a separate allocation from the
Entry's own pointer word, so the&selfborrow (which covers only the pointer word) does not forbid mutating the pointee; the raw pointer carries write provenance and no overlapping&/&mutview is formed. Non-atomic is sound because a shard's entries are owned by exactly one core and never sent across threads (ADR-0002/0005; the store is!Send/!Sync). The freq is a bounded 0..=3 counter, not a clock (ADR-0003 holds).Tests
Eviction is BYTE-IDENTICAL: all 17 eviction integration tests pass unchanged (incl.
evict_to_fit_is_deterministic_on_replay, the pool-eviction determinism + hot-key-survival tests) + 2 new unit tests asserting the shared bump equals the mutable bump across Str / Str+TTL / Coll arms + a test bumping freq through a plain&Entry. 1156 server+ironcache tests pass. GET microbench delta[+0.21% +0.70% +1.23%](criterion: within noise). clippy-D warnings+ io_uring build + invariant + prior-art lints + fmt clean; no dashes.Part of #576 (per-slot Arc-COW prerequisite).
🤖 Generated with Claude Code