Skip to content

refactor: extract the commit protocol (3-fsync sequence) into commit.rs — extraction PR 5 - #79

Merged
Xof merged 1 commit into
mainfrom
feature/transaction-commit-protocol
Jun 23, 2026
Merged

refactor: extract the commit protocol (3-fsync sequence) into commit.rs — extraction PR 5#79
Xof merged 1 commit into
mainfrom
feature/transaction-commit-protocol

Conversation

@Xof

@Xof Xof commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Task 5 of the transaction.rs extraction (after #76, #77, #78). Lifts the 3-fsync commit protocol — the body of commit_inner — out of lifecycle.rs into a stateless function-module transaction/commit.rs. The most durability-critical sequence in the engine; moved VERBATIM, only field-access paths changed.

What changed

  • commit.rs (new): CommitCtx<'a> bundles the ten pieces of manager state the protocol touches (cache as &RefCell, the rest &mut), and run_commit(ctx) holds the load-bearing sequence: I27 savepoint-freed flatten → I28 pre-drain flush → freemap persist → main flush → txn_counter bump → superblock build → write to the inactive slot → superblock fsync → roots/packer/freemap promotion. Stateless — owns nothing.
  • commit_inner (lifecycle.rs) is now a thin caller constructing the ctx. The public commit() poison-on-error wrapper is unchanged.
  • The dead TransactionManager::persist_freemap wrapper (its only caller was commit_inner) was removed; run_commit inlines freemap.persist(...) directly, and the wrapper's "Step 0" reasoning was relocated into that scope.

Why CommitCtx takes cache: &RefCell<PageCache>

The protocol deliberately drops and re-acquires the cache borrow three times (the I28 pre-drain flush and persist each borrow-and-release before the held borrow for steps 1-4). Passing the RefCell preserves the original borrow lifetimes — a single &mut PageCache would force one continuous borrow and persist's own borrow_mut would panic. The three borrow scopes stay separate.

Verification

  • cargo test578 passing, 0 failed (273 chisel-lib) — identical to the pre-refactor baseline.
  • Load-bearing oracle: tests/spillway_integration.rs fsync_delta == 3 (I28 pre-drain + main + superblock) — green; the I18 guardrail persist_freemap_does_not_reuse_committed_live_pages — green.
  • cargo clippy --workspace --all-targets -- -D warnings clean (confirms no dead code from the removed wrapper); cargo fmt --check clean; pytest119 passing.
  • The controller verified run_commit statement-by-statement against the original commit_inner: byte-identical ordering, only self.Xctx.X plus deref-assigns, all invariant comments (I27/I28/I119/the step descriptions) preserved verbatim.

Next: Task 6 (StagingTxn — the BUG#2 atomic allocate_inner prepare/install), the final extraction, one PR off updated main.

@Xof
Xof merged commit 3102a5d into main Jun 23, 2026
6 checks passed
@Xof
Xof deleted the feature/transaction-commit-protocol branch June 23, 2026 04:03
@github-actions

Copy link
Copy Markdown

🚦 Bench results: PR vs main

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

Scenario Mode Δ throughput Worst Δ
ycsb-b chisel-mem -6.8% p95 +19.4% ⚠️
ycsb-b redb-strict +0.4% p99 +16.1% ⚠️
mutation-log chisel-strict -0.9% p50 +11.5% ⚠️
document-store chisel-mem +2.2%
document-store chisel-strict +0.4%
document-store redb-strict +3.5%
document-store sqlite-strict +0.3%
mutation-log chisel-mem -0.3%
mutation-log redb-strict +0.1%
mutation-log sqlite-strict -0.9%
ycsb-a chisel-mem +0.2%
ycsb-a chisel-strict -1.1%
ycsb-a redb-strict -0.1%
ycsb-a sqlite-strict +1.5%
ycsb-b chisel-strict -0.5%
ycsb-b sqlite-strict +1.0%
Per-scenario detail (4 metrics × cells)

document-store

Mode Throughput p50 p95 p99
chisel-mem 19850 ops/s → 20282 ops/s (+2.2%) 13.6 µs → 13.3 µs (-2.0%) 134.6 µs → 131.0 µs (-2.7%) 617.5 µs → 591.4 µs (-4.2%)
chisel-strict 2769 ops/s → 2781 ops/s (+0.4%) 20.9 µs → 20.6 µs (-1.7%) 1.13 ms → 1.14 ms (+0.6%) 2.95 ms → 2.88 ms (-2.2%)
redb-strict 3998 ops/s → 4137 ops/s (+3.5%) 18.1 µs → 18.1 µs (-0.1%) 635.8 µs → 583.9 µs (-8.2%) 1.61 ms → 1.50 ms (-6.7%)
sqlite-strict 5105 ops/s → 5120 ops/s (+0.3%) 20.5 µs → 20.6 µs (+0.3%) 441.7 µs → 440.1 µs (-0.4%) 1.41 ms → 1.36 ms (-3.5%)

mutation-log

Mode Throughput p50 p95 p99
chisel-mem 52977 ops/s → 52837 ops/s (-0.3%) 22.8 µs → 22.8 µs (-0.3%) 26.4 µs → 26.6 µs (+0.7%) 35.2 µs → 35.7 µs (+1.3%)
chisel-strict 1689 ops/s → 1674 ops/s (-0.9%) 433.6 µs → 483.5 µs (+11.5%) ⚠️ 932.2 µs → 960.9 µs (+3.1%) 8.63 ms → 7.88 ms (-8.7%)
redb-strict 1804 ops/s → 1806 ops/s (+0.1%) 186.7 µs → 191.4 µs (+2.5%) 306.1 µs → 328.7 µs (+7.4%) 30.47 ms → 30.00 ms (-1.6%)
sqlite-strict 4559 ops/s → 4518 ops/s (-0.9%) 131.4 µs → 132.9 µs (+1.1%) 360.5 µs → 365.2 µs (+1.3%) 531.3 µs → 524.5 µs (-1.3%)

ycsb-a

Mode Throughput p50 p95 p99
chisel-mem 40825 ops/s → 40908 ops/s (+0.2%) 38.3 µs → 38.3 µs (+0.1%) 55.3 µs → 56.9 µs (+2.8%) 72.1 µs → 73.4 µs (+1.8%)
chisel-strict 2133 ops/s → 2110 ops/s (-1.1%) 334.0 µs → 336.9 µs (+0.9%) 1.04 ms → 1.06 ms (+2.1%) 3.01 ms → 2.93 ms (-2.7%)
redb-strict 2683 ops/s → 2681 ops/s (-0.1%) 149.4 µs → 151.3 µs (+1.3%) 277.0 µs → 283.0 µs (+2.2%) 555.8 µs → 495.3 µs (-10.9%)
sqlite-strict 148340 ops/s → 150527 ops/s (+1.5%) 6.8 µs → 6.8 µs (-0.3%) 8.8 µs → 8.6 µs (-1.4%) 12.9 µs → 10.7 µs (-17.0%)

ycsb-b

Mode Throughput p50 p95 p99
chisel-mem 177908 ops/s → 165819 ops/s (-6.8%) ⚠️ 4.2 µs → 4.2 µs (-1.4%) 40.2 µs → 48.0 µs (+19.4%) ⚠️ 46.4 µs → 54.6 µs (+17.6%) ⚠️
chisel-strict 19779 ops/s → 19678 ops/s (-0.5%) 6.1 µs → 6.0 µs (-2.5%) 352.8 µs → 359.1 µs (+1.8%) 931.2 µs → 938.2 µs (+0.8%)
redb-strict 26932 ops/s → 27033 ops/s (+0.4%) 3.0 µs → 2.8 µs (-5.4%) 151.6 µs → 159.2 µs (+5.0%) 245.4 µs → 284.9 µs (+16.1%) ⚠️
sqlite-strict 179653 ops/s → 181446 ops/s (+1.0%) 5.6 µs → 5.5 µs (-1.6%) 7.6 µs → 7.5 µs (-1.4%) 10.1 µs → 9.9 µs (-2.0%)
Generated by chisel-bench-diff at 2026-06-23T04:17:58Z. 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