Skip to content

refactor: extract SlotPacker (R1 live-slot packing) — extraction PR 2 - #77

Merged
Xof merged 2 commits into
mainfrom
feature/transaction-slotpacker
Jun 23, 2026
Merged

refactor: extract SlotPacker (R1 live-slot packing) — extraction PR 2#77
Xof merged 2 commits into
mainfrom
feature/transaction-slotpacker

Conversation

@Xof

@Xof Xof commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Task 3 of the transaction.rs extraction (after #76). Extracts SlotPacker — an owned state unit for the R1 live-slot / insert-cursor model — out of TransactionManager. Pure behavior-preserving refactor; the existing suite is the oracle.

What changed

  • SlotPacker (in transaction/packing.rs) now owns the three fields committed_live_slots / current_live_slots / insert_cursor and 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 outside packing.rs touches those fields directly (surface-checked).
  • Borrow crux: SlotPacker::insert holds &mut self.packer while needing a fresh data page, whose allocation touches the disjoint freemap field cluster. The freemap take/cow_alloc/put dance is hoisted to the caller (insert_into_data_page) around the alloc closure — the same disjoint-field-borrow pattern ht_insert already uses. alloc receives cache as a param, captures only the freemap field refs.
  • Three plan-interface gaps resolved from the actual code: insert takes a packing_enabled bool (SlotPacker stays ignorant of savepoints); release returns Option<u64> so the caller owns the txn_freed_pages push; from_committed seeds the open-time scan (vs empty new).
  • The now-dead TransactionManager::allocate_data_page (its sole caller inlined the dance) was removed; its cross-file comment references were reconciled to point at cow_alloc (second commit) — including three that were already loose pre-refactor (persist_freemap allocates freemap pages via structural_extend, never data pages).

Verification

  • cargo test578 passing, 0 failed (273 chisel-lib) — identical to the pre-refactor baseline
  • cargo clippy --workspace --all-targets -- -D warnings clean; cargo fmt --check clean
  • pytest119 passing (binding untouched)
  • Surface check: the three packer fields appear outside packing.rs only as scan locals, the Savepoint struct'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.

Xof added 2 commits June 22, 2026 19:45
…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.
@Xof
Xof merged commit 7e2270c into main Jun 23, 2026
10 checks passed
@Xof
Xof deleted the feature/transaction-slotpacker branch June 23, 2026 03:00
@github-actions

Copy link
Copy Markdown

🚦 Bench results: PR vs main

⚠️ 3 regression(s) detected across 2 scenario/mode pair(s)

Scenario Mode Δ throughput Worst Δ
mutation-log sqlite-strict +0.0% p99 +11.7% ⚠️
ycsb-a chisel-mem -7.9% p50 +8.3% ⚠️
document-store chisel-mem +1.3%
document-store chisel-strict +5.5%
document-store redb-strict +3.1%
document-store sqlite-strict +0.4%
mutation-log chisel-mem -4.6%
mutation-log chisel-strict -0.1%
mutation-log redb-strict -0.2%
ycsb-a chisel-strict +0.1%
ycsb-a redb-strict +0.1%
ycsb-a sqlite-strict -0.2%
ycsb-b chisel-mem +0.2%
ycsb-b chisel-strict -2.1%
ycsb-b redb-strict +0.2%
ycsb-b sqlite-strict +0.6%
Per-scenario detail (4 metrics × cells)

document-store

Mode Throughput p50 p95 p99
chisel-mem 18530 ops/s → 18777 ops/s (+1.3%) 14.9 µs → 14.8 µs (-0.8%) 137.9 µs → 136.2 µs (-1.2%) 671.6 µs → 655.0 µs (-2.5%)
chisel-strict 2938 ops/s → 3099 ops/s (+5.5%) 24.7 µs → 22.8 µs (-7.6%) 908.4 µs → 819.5 µs (-9.8%) 3.14 ms → 2.87 ms (-8.8%)
redb-strict 4009 ops/s → 4132 ops/s (+3.1%) 12.5 µs → 12.0 µs (-3.7%) 511.3 µs → 439.2 µs (-14.1%) 1.39 ms → 1.26 ms (-9.5%)
sqlite-strict 5131 ops/s → 5152 ops/s (+0.4%) 21.2 µs → 20.9 µs (-1.4%) 363.0 µs → 345.2 µs (-4.9%) 1.33 ms → 1.29 ms (-3.2%)

mutation-log

Mode Throughput p50 p95 p99
chisel-mem 51669 ops/s → 49293 ops/s (-4.6%) 23.0 µs → 23.1 µs (+0.3%) 28.7 µs → 29.6 µs (+3.2%) 35.8 µs → 39.2 µs (+9.2%)
chisel-strict 1773 ops/s → 1771 ops/s (-0.1%) 296.7 µs → 296.0 µs (-0.2%) 659.4 µs → 649.1 µs (-1.6%) 17.96 ms → 18.29 ms (+1.8%)
redb-strict 1806 ops/s → 1802 ops/s (-0.2%) 146.5 µs → 147.8 µs (+0.9%) 231.8 µs → 243.4 µs (+5.0%) 34.40 ms → 33.57 ms (-2.4%)
sqlite-strict 5031 ops/s → 5032 ops/s (+0.0%) 97.2 µs → 96.9 µs (-0.4%) 267.4 µs → 270.2 µs (+1.1%) 370.1 µs → 413.3 µs (+11.7%) ⚠️

ycsb-a

Mode Throughput p50 p95 p99
chisel-mem 37710 ops/s → 34741 ops/s (-7.9%) ⚠️ 41.0 µs → 44.4 µs (+8.3%) ⚠️ 60.1 µs → 64.5 µs (+7.2%) 77.1 µs → 83.8 µs (+8.7%)
chisel-strict 2262 ops/s → 2263 ops/s (+0.1%) 255.6 µs → 256.2 µs (+0.2%) 743.7 µs → 724.2 µs (-2.6%) 1.58 ms → 1.71 ms (+8.2%)
redb-strict 2681 ops/s → 2683 ops/s (+0.1%) 116.3 µs → 116.0 µs (-0.3%) 202.2 µs → 196.6 µs (-2.8%) 389.7 µs → 326.9 µs (-16.1%)
sqlite-strict 132941 ops/s → 132625 ops/s (-0.2%) 7.6 µs → 7.7 µs (+0.8%) 10.1 µs → 10.1 µs (+0.3%) 11.8 µs → 12.2 µs (+2.9%)

ycsb-b

Mode Throughput p50 p95 p99
chisel-mem 162521 ops/s → 162861 ops/s (+0.2%) 4.7 µs → 4.7 µs (-1.1%) 42.8 µs → 43.8 µs (+2.2%) 49.8 µs → 49.4 µs (-0.7%)
chisel-strict 21842 ops/s → 21381 ops/s (-2.1%) 7.5 µs → 7.1 µs (-6.4%) 263.8 µs → 262.2 µs (-0.6%) 669.5 µs → 706.3 µs (+5.5%)
redb-strict 27223 ops/s → 27269 ops/s (+0.2%) 3.1 µs → 3.1 µs (-0.3%) 119.5 µs → 116.2 µs (-2.8%) 191.0 µs → 178.5 µs (-6.6%)
sqlite-strict 161560 ops/s → 162600 ops/s (+0.6%) 6.3 µs → 6.3 µs (-0.9%) 8.4 µs → 8.3 µs (-1.1%) 10.6 µs → 10.6 µs (+0.4%)
Generated by chisel-bench-diff at 2026-06-23T03:12:33Z. Compares PR HEAD against main. Never blocks merge — signal, not gate. Thresholds: throughput 5%, p50 5%, p95 10%, p99 10%.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant