bench: restore the rows a removed cache ceiling was still excluding (#100) - #135
Merged
Merged
Conversation
BENCH-5. micro_grid skipped cells and dropped two whole rows on a
premise that no longer holds. TX_BUDGET_BYTES was justified as avoiding
"Chisel's CacheFull ... cache hard ceiling is ~16 MB", and
update-1000pertx / delete-1000pertx were never registered because "1000
random updates/deletes pin ~1000 distinct dirty data pages, exceeding
Chisel's 2048-page cache ceiling. The cells are not measurable under
default cache settings."
Both halves are stale. CACHE_SIZE_PAGES is 256, not 2048, and the
strict cache ceiling itself went away when the bench engines gained the
spillway at production-default scale — runner.rs already says so
("there is no strict cache-page ceiling on transaction size") and
chisel_engine.rs sets `spillway_max_bytes(cache_max_bytes * 1024)`.
Measured every excluded cell against the current engine rather than
assuming either way. All of them complete:
allocate-1000pertx 16KB 0.71 s update-1000pertx 16KB 1.27 s
allocate-1000pertx 128KB 2.70 s delete-1000pertx 16KB 0.80 s
allocate-1000pertx 1MB 18.51 s (smaller sizes: ~0.5 s)
So capacity is no longer the constraint — wall clock is, for exactly
one cell. TX_BUDGET_BYTES is kept but reframed as a bench-runtime
budget and raised to 128 MiB: that admits the 16KB and 128KB columns,
which were losing real coverage for no reason, and still excludes
1MB x 1000, which at ~18.5 s per iteration across five engine modes
would add hours to every run. Deleting the constant outright, as the
issue suggests, is the one part of its direction not followed, and the
comment now says why with the measurement behind it.
update-1000pertx and delete-1000pertx are registered.
BENCH-6. The module header claimed a "270-cell micro grid ... 9 row
groups" while six rows registered 165 cells, so anyone checking that a
run completed would conclude 105 cells had silently failed. The header
now states 230 cells across 8 rows with the per-row breakdown, and
runner.rs's "8 of 9 rows: rows 1, 2, 4-9" is replaced by what actually
distinguishes them (every row except read-warm, which reuses one engine
and has its own capture function). Verified 230 both by deriving it
from the registration rules — the same derivation reproduces the old
165 against the old constants — and by counting
`cargo bench --bench micro_grid -- --list`. The dead `seed_for` arms
for the two rows are now live again rather than removed.
BENCH-7. cross-engine.md's methodology footer told readers "Each engine
takes a single fsync per commit". Chisel performs three (pre-drain,
data pages, superblock), which the bench's own runner comments state.
This renderer's output is described as suitable for the README and
release notes, so the claim understated Chisel's per-commit cost by 3x
in published material and implied the three commit paths were
equivalent. The footer now states each engine's protocol, since that
asymmetry is what the throughput table is read against.
Closes #100.
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.
Closes #100 (3 findings, DESIGN/docs).
BENCH-5 — coverage lost to a premise that no longer holds
micro_gridskipped cells and dropped two whole rows:TX_BUDGET_BYTES = 8 MiB, justified as avoiding "Chisel's CacheFull at large1000-per-tx writes (cache hard ceiling is ~16 MB)"
update-1000pertxanddelete-1000pertxnever registered, because "1000random updates/deletes pin ~1000 distinct dirty data pages, exceeding
Chisel's 2048-page cache ceiling. The cells are not measurable under default
cache settings."
Both halves are stale.
CACHE_SIZE_PAGESis 256, not 2048, and the strictcache ceiling went away when the bench engines gained the spillway at
production-default scale —
runner.rsalready says "there is no strictcache-page ceiling on transaction size" and
chisel_engine.rssetsspillway_max_bytes(cache_max_bytes * 1024).Measured rather than assumed
Every excluded cell, against the current engine at
CACHE_SIZE_PAGES:All complete. Capacity is no longer the constraint — wall clock is, for exactly
one cell.
What I did, and where I deviated
TX_BUDGET_BYTESis kept but reframed as a bench-runtime budget and raisedto 128 MiB. That admits the 16KB and 128KB columns (real coverage lost for no
reason) and still excludes 1MB × 1000: at ~18.5 s per iteration across five
engine modes, that single cell would add hours to every micro-grid run.
The issue's direction says to delete
TX_BUDGET_BYTESoutright. That is theone part I did not follow, and the comment now records the measurement behind
the decision so it can be revisited on evidence rather than re-derived. If you
want the 1MB × 1000 column too, say so and I'll drop the constant — it is a
one-line change.
update-1000pertxanddelete-1000pertxare registered.BENCH-6 — the documented grid size was off by 105 cells
The header claimed a "270-cell micro grid ... 9 row groups"; six rows
registered 165 cells. A reader checking that a run completed would conclude 105
cells had silently failed — and the live-but-uncalled
seed_forarms for thetwo missing rows reinforced the illusion that they ran.
The header now states 230 cells across 8 rows with the per-row breakdown,
and
runner.rs's "8 of 9 rows: rows 1, 2, 4–9" is replaced by what actuallydistinguishes them (every row except
read-warm, which reuses one engine andhas its own capture function).
Verified two independent ways:
old 165 against the old constants, which is what validates the model.
cargo bench --bench micro_grid -- --list→ 230.The
seed_forarms are now live again rather than deleted.BENCH-7 — the published methodology footer understated Chisel 3×
cross-engine.md's footer told readers "Each engine takes a single fsync percommit through the disk write cache." Chisel performs three (pre-drain,
data pages, superblock) — as
src/lib.rs,lifecycle.rsand the bench's ownrunner.rsall state.This renderer's module doc describes its output as suitable for the README and
1.0 release notes, so the wrong claim propagates into public performance
material, and it invites the reader to assume the three commit paths are
equivalent when Chisel is doing strictly more syscall work for the same
throughput number. The footer now states each engine's protocol: Chisel's three
fsyncs, redb's
Durability::Immediate, and SQLite's WAL +synchronous=FULL+fullfsync=ON.Note on runtime
Re-registering the two rows and widening the budget takes the grid from 165 to
230 cells, so a full
cargo bench --bench micro_gridwill take meaningfullylonger. That is the coverage the issue is about restoring, but it is a real
cost worth being aware of.
bench/results/is untracked, so no committedartifact goes stale.
Verification
cargo testall green,cargo clippy --workspace --all-targets -- -D warningsclean,
cargo fmt --checkclean.