refactor: extract SlotPacker (R1 live-slot packing) — extraction PR 2 - #77
Merged
Conversation
…tion The SlotPacker extraction inlined the freemap take/cow_alloc/put dance into insert_into_data_page's caller (the borrow checker forbids calling the &mut self allocate_data_page inside the packer-insert closure), making allocate_data_page dead code that clippy -D warnings rejects. Removing it left comments across page_cache.rs, defrag.rs, transaction/mod.rs, lifecycle.rs, and tests.rs naming a deleted symbol. They now point at the real allocator (cow_alloc) reached via the hoist. Three of them (lifecycle.rs, two in tests.rs) were already loose pre-refactor: persist_freemap allocates freemap pages via structural_extend, never the data-page allocator — corrected here as well.
🚦 Bench results: PR vs main
Per-scenario detail (4 metrics × cells)document-store
mutation-log
ycsb-a
ycsb-b
|
This was referenced Jun 23, 2026
Merged
Xof
added a commit
that referenced
this pull request
Jun 23, 2026
…ion deferred (I141) (#80) The 2026-06-22 review's god-module SMELL was worked through four unit extractions (SlotPacker #77, FreemapRecycle #78, CommitProtocol #79, FaultInjector #76); the final StagingTxn extraction is deliberately deferred. The candidate-prepare/install staging vocabulary is shared across allocate_inner (staging.rs) and update_inner/delete_inner (mutate.rs), so a context-based extraction cannot be contained to staging.rs without dragging the delicate mutation paths through a mechanical wrapper change. Recorded as future work to be done incrementally if/when those paths are touched, not re-triggered from the SMELL alone.
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.
Task 3 of the
transaction.rsextraction (after #76). ExtractsSlotPacker— an owned state unit for the R1 live-slot / insert-cursor model — out ofTransactionManager. Pure behavior-preserving refactor; the existing suite is the oracle.What changed
SlotPacker(intransaction/packing.rs) now owns the three fieldscommitted_live_slots/current_live_slots/insert_cursorand the packing logic. Narrow interface:new/from_committed,insert(cache, alloc, packing_enabled, value),release -> Option<u64>,begin/commit/rollback,snapshot/restore/clear_cursor, plus read accessors. No code outsidepacking.rstouches those fields directly (surface-checked).SlotPacker::insertholds&mut self.packerwhile needing a fresh data page, whose allocation touches the disjoint freemap field cluster. The freemaptake/cow_alloc/putdance is hoisted to the caller (insert_into_data_page) around the alloc closure — the same disjoint-field-borrow patternht_insertalready uses.allocreceivescacheas a param, captures only the freemap field refs.inserttakes apacking_enabledbool (SlotPacker stays ignorant of savepoints);releasereturnsOption<u64>so the caller owns thetxn_freed_pagespush;from_committedseeds the open-time scan (vs emptynew).TransactionManager::allocate_data_page(its sole caller inlined the dance) was removed; its cross-file comment references were reconciled to point atcow_alloc(second commit) — including three that were already loose pre-refactor (persist_freemapallocates freemap pages viastructural_extend, never data pages).Verification
cargo test→ 578 passing, 0 failed (273 chisel-lib) — identical to the pre-refactor baselinecargo clippy --workspace --all-targets -- -D warningsclean;cargo fmt --checkcleanpytest→ 119 passing (binding untouched)packing.rsonly as scan locals, theSavepointstruct's own snapshot fields, and comments.Reviewed
Call-site rewires (recovery/lifecycle/savepoints/stats/tests) independently verified semantically faithful; the freemap-hoist error-path ordering and all-three-lifecycle-clear-cursor invariants confirmed preserved.
Next: Task 4 (FreemapRecycle — the hardest), one PR off updated
main.