Skip to content

docs: commenting pass — 5 invariant comments + 7 wrong comments memorialized - #83

Merged
Xof merged 1 commit into
mainfrom
comment-pass-2026-06-23
Jun 23, 2026
Merged

docs: commenting pass — 5 invariant comments + 7 wrong comments memorialized#83
Xof merged 1 commit into
mainfrom
comment-pass-2026-06-23

Conversation

@Xof

@Xof Xof commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

A LEAVE-default /comment-run pass over the engine (11 parallel agents, one per subsystem). The codebase is already heavily commented from three prior passes, so the overwhelming verdict was LEAVE — this PR is 5 high-value additions where a tradeoff/invariant lived only in a test or a callee:

  • page_cache.rsdirty_count counts ONLY in-cache dirty entries, never spilled pages; that's what makes the dirty_count == entries.len() eviction short-circuit sound.
  • overflow.rs — the wrong-type + zero-length guards MUST precede Vec::with_capacity, so an untrusted total_length can't drive a speculative giant allocation.
  • membership_index.rs — pack the POST-insert inner.depth, not the stale pre-insert depth (invisible at depth 0; pinned by inner_grow_roundtrip).
  • transaction/fault.rsfail_next_handle_table_op is consumed in handle_table_insert_candidate, which both allocate_inner and update_inner route through.
  • python/errors.rs — add TagMismatchError to the exception-hierarchy doc.

All are ADD/EXTEND (EXTEND appends, never overwrites). Verified: clippy --all-targets clean, fmt clean, maturin develop + pytest 119 pass.

0 bugs found. The pass also surfaced 7 actively-wrong comments (mismatches, not gaps) — left UNCHANGED per the discipline (memorialize, don't paper over), memorialized for triage. They're mostly comment-rot from the freemap rewrite + the transaction.rs extraction (e.g. page_io.rs PageIo::fsync still says "twice per commit" but it's 3 fsyncs now; defrag.rs doc says "net drop" but the code is a set-difference). See the follow-up.

…left implicit

A LEAVE-default pass over the engine (already heavily commented by three
prior passes). Five high-value additions where a tradeoff/invariant lived
only in a test or a callee:

- page_cache.rs: dirty_count counts ONLY in-cache dirty entries, never
  spilled pages — that is what makes the dirty_count==entries.len()
  eviction short-circuit sound.
- overflow.rs: the wrong-type + zero-length guards MUST precede the
  Vec::with_capacity, so an untrusted total_length can't drive a
  speculative giant allocation.
- membership_index.rs: pack the POST-insert inner.depth, not the stale
  pre-insert depth (invisible at depth 0; pinned by inner_grow_roundtrip).
- transaction/fault.rs: fail_next_handle_table_op is consumed in
  handle_table_insert_candidate, which both allocate_inner and
  update_inner route through.
- python/errors.rs: add TagMismatchError to the exception-hierarchy doc.
@github-actions

Copy link
Copy Markdown

🚦 Bench results: PR vs main

✅ No regressions detected

Scenario Mode Δ throughput Worst Δ
document-store chisel-mem -0.3%
document-store chisel-strict -2.6%
document-store redb-strict +0.2%
document-store sqlite-strict +0.2%
mutation-log chisel-mem +0.4%
mutation-log chisel-strict +3.5%
mutation-log redb-strict +0.2%
mutation-log sqlite-strict -0.6%
ycsb-a chisel-mem -3.2%
ycsb-a chisel-strict +5.1%
ycsb-a redb-strict +0.3%
ycsb-a sqlite-strict -0.3%
ycsb-b chisel-mem -1.3%
ycsb-b chisel-strict +12.3%
ycsb-b redb-strict -0.1%
ycsb-b sqlite-strict +1.4%
Per-scenario detail (4 metrics × cells)

document-store

Mode Throughput p50 p95 p99
chisel-mem 20078 ops/s → 20024 ops/s (-0.3%) 13.4 µs → 13.4 µs (-0.1%) 133.6 µs → 133.7 µs (+0.1%) 603.8 µs → 605.8 µs (+0.3%)
chisel-strict 2546 ops/s → 2481 ops/s (-2.6%) 20.5 µs → 20.5 µs (+0.3%) 1.49 ms → 1.51 ms (+1.6%) 2.77 ms → 2.85 ms (+2.8%)
redb-strict 4009 ops/s → 4017 ops/s (+0.2%) 18.7 µs → 18.3 µs (-2.5%) 740.3 µs → 727.1 µs (-1.8%) 1.89 ms → 1.76 ms (-6.7%)
sqlite-strict 5084 ops/s → 5097 ops/s (+0.2%) 20.8 µs → 20.4 µs (-2.0%) 573.6 µs → 576.0 µs (+0.4%) 1.51 ms → 1.51 ms (-0.3%)

mutation-log

Mode Throughput p50 p95 p99
chisel-mem 53583 ops/s → 53776 ops/s (+0.4%) 22.6 µs → 22.4 µs (-0.7%) 26.0 µs → 26.4 µs (+1.5%) 35.0 µs → 35.1 µs (+0.3%)
chisel-strict 1550 ops/s → 1604 ops/s (+3.5%) 626.5 µs → 586.9 µs (-6.3%) 1.29 ms → 1.22 ms (-5.2%) 2.96 ms → 3.02 ms (+2.0%)
redb-strict 1802 ops/s → 1806 ops/s (+0.2%) 226.9 µs → 227.1 µs (+0.1%) 447.3 µs → 433.2 µs (-3.2%) 25.82 ms → 26.23 ms (+1.6%)
sqlite-strict 4084 ops/s → 4060 ops/s (-0.6%) 143.8 µs → 146.8 µs (+2.1%) 480.5 µs → 488.9 µs (+1.8%) 894.5 µs → 827.0 µs (-7.5%)

ycsb-a

Mode Throughput p50 p95 p99
chisel-mem 41321 ops/s → 40016 ops/s (-3.2%) 38.4 µs → 39.5 µs (+3.0%) 54.2 µs → 55.3 µs (+2.0%) 70.5 µs → 73.8 µs (+4.8%)
chisel-strict 1901 ops/s → 1998 ops/s (+5.1%) 363.2 µs → 354.4 µs (-2.4%) 1.42 ms → 1.30 ms (-9.0%) 2.40 ms → 2.34 ms (-2.7%)
redb-strict 2674 ops/s → 2682 ops/s (+0.3%) 186.9 µs → 188.4 µs (+0.8%) 364.6 µs → 367.6 µs (+0.8%) 937.7 µs → 884.4 µs (-5.7%)
sqlite-strict 149430 ops/s → 148995 ops/s (-0.3%) 6.8 µs → 6.8 µs (+0.1%) 8.7 µs → 8.8 µs (+1.0%) 12.0 µs → 11.4 µs (-5.5%)

ycsb-b

Mode Throughput p50 p95 p99
chisel-mem 182533 ops/s → 180157 ops/s (-1.3%) 4.2 µs → 4.2 µs (+1.0%) 38.6 µs → 39.2 µs (+1.6%) 44.6 µs → 45.9 µs (+2.9%)
chisel-strict 17112 ops/s → 19225 ops/s (+12.3%) 6.0 µs → 5.7 µs (-4.2%) 390.7 µs → 358.7 µs (-8.2%) 1.17 ms → 1.08 ms (-7.3%)
redb-strict 27154 ops/s → 27137 ops/s (-0.1%) 2.8 µs → 2.7 µs (-4.2%) 188.1 µs → 184.6 µs (-1.9%) 301.1 µs → 306.2 µs (+1.7%)
sqlite-strict 180518 ops/s → 183110 ops/s (+1.4%) 5.6 µs → 5.5 µs (-0.6%) 7.5 µs → 7.4 µs (-0.8%) 9.8 µs → 9.7 µs (-1.2%)
Generated by chisel-bench-diff at 2026-06-23T18:57:58Z. Compares PR HEAD against main. Never blocks merge — signal, not gate. Thresholds: throughput 5%, p50 5%, p95 10%, p99 10%.

@Xof
Xof merged commit 55e83c3 into main Jun 23, 2026
10 checks passed
Xof added a commit that referenced this pull request Jul 1, 2026
Resolve the sole conflict in src/page_io.rs: main's commenting pass
(#83/#84) corrected the fsync doc-comment from "twice" to "three times
per commit" (I28 pre-drain flush + data pages + superblock), while this
branch renamed the Memory backing field pages -> bytes. Kept both: the
corrected "three times" comment (still accurate under encryption — the
per-page seal changes the unit size, not the fsync count) and a match
pattern valid for the renamed field (`Backing::Memory { .. }`).

The other three shared files (python/src/errors.rs, src/defrag.rs,
src/page_cache.rs) auto-merged cleanly. Verified on the merged tree:
680 tests across 32 suites pass, clippy clean on lib + chisel-py.
@Xof
Xof deleted the comment-pass-2026-06-23 branch July 1, 2026 22:21
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