perf: optimize bus balance checking - #2189
Conversation
checkShards ran every constraint against each shard and then discarded any bus failures from the results. That is wasteful twice over: a lone shard normally does not balance, so each bus took the failure path and called count() twice -- two extra full scans of every port -- purely to build a Failure that was then thrown away. The buses were then tallied again across all the shards. For a three-shard set that is roughly twelve scans where three suffice. Filter the constraints instead of the results. getBusAndOtherConstraints sorts a schema's constraints into buses and non-buses in the single walk that was already needed to find the buses, and the new schema.AcceptsSubset runs only the non-bus ones against each shard. The buses are then judged across all shards as before. Also rename checkShardGroup to checkShards and drop "group" from the surrounding names, since a bus balances over the complete set of shards and never over a subset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Olivier Bégassat <olivier.begassat.cours@gmail.com>
The three row loops over a bus port all re-fetched the port's selector column, and each of its message columns, on every single row. For a port of height H holding a two-column message that is 3H column lookups where 3 will do. Introduce portColumns, which resolves a port's selector and message columns once, and hoist it out of the loops in accumulate, count and Failure.requiredCellsOfPort. Note the per-row Get calls remain, and they dominate the loop, so this is a modest constant-factor win rather than a dramatic one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Olivier Bégassat <olivier.begassat.cours@gmail.com>
NetTally returned one trace's signed tally so a harness could sum shard tallies and look for non-zeros. That design was never wired up: checkShards calls AcceptsGroup over all shards at once, which tallies them straight into a single map. Nothing in the repo ever called NetTally. Note this is not the failure-path rescan, which is count() and remains in place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Olivier Bégassat <olivier.begassat.cours@gmail.com>
alpha/beta said nothing about what either side of the bus does. Rename them to sndr/rcvr across the .lisp sources and every trace file, which must move together or the traces no longer resolve against their modules. Also separate the trace groups in the sharded fixtures with blank lines, which the reader ignores, purely so the long lines are easier to tell apart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Olivier Bégassat <olivier.begassat.cours@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes sharded trace checking and bus-balance validation by avoiding per-shard bus evaluation and reducing repeated column lookups during bus scans, while also cleaning up unused bus APIs and updating related fixtures.
Changes:
- Avoids running bus constraints per-shard by splitting constraints into bus vs non-bus once (
getBusAndOtherConstraints) and addingschema.AcceptsSubsetfor checking only non-bus constraints on each shard. - Speeds up bus constraint/failure rescans by resolving a port’s selector/message columns once per port (
portColumns) rather than per row. - Removes unused
NetTallyand renames fixture modules (alpha/beta→sndr/rcvr) to match updated test expectations.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/corset/valid/bus_05.shards.accepts | Adds spacing between shard-set cases for readability (no semantic change). |
| testdata/corset/valid/bus_02.rejects | Updates fixture column names to sndr/rcvr. |
| testdata/corset/valid/bus_02.lisp | Renames fixture modules to sndr/rcvr. |
| testdata/corset/valid/bus_02.accepts | Updates fixture column names to sndr/rcvr. |
| testdata/corset/valid/bus_01.shards.rejects | Updates shard fixture column names and adds spacing between cases. |
| testdata/corset/valid/bus_01.shards.accepts | Updates shard fixture column names and adds spacing between cases. |
| testdata/corset/valid/bus_01.rejects | Updates fixture column names to sndr/rcvr. |
| testdata/corset/valid/bus_01.lisp | Renames fixture modules to sndr/rcvr. |
| testdata/corset/valid/bus_01.accepts | Updates fixture column names to sndr/rcvr. |
| pkg/test/util/check_legacy.go | Refactors sharded checking to avoid per-shard bus evaluation; introduces constraint partitioning and uses AcceptsSubset. |
| pkg/schema/schemas.go | Adds AcceptsSubset to run a provided iterator of constraints against a trace. |
| pkg/schema/constraint/bus/failure.go | Uses cached per-port column lookups when collecting required cells on failure. |
| pkg/schema/constraint/bus/constraint.go | Removes unused NetTally; adds portColumns and uses it to reduce repeated column lookups in tally/count paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The balance check walks every selected row of every bus port, hashing the row's message and updating a tally. Nothing measured that loop, so the cost of each part of it was guesswork. Benchmark AcceptsGroup over a synthetic one-sender / one-receiver bus across four dimensions, each of which isolates one question: row count; how many distinct messages those rows carry (how large the tally grows, and how often a row hits an existing key); balanced versus unbalanced (the latter also pays the count() rescans on the failure path); and koalabear versus bls12_377, which hash an element with one multiply and four respectively. Note this is the repo's first Go benchmark. The corset fixture harness cannot serve here, since it spends most of its time compiling constraints and so cannot see a per-row cost. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Olivier Bégassat <olivier.begassat.cours@gmail.com>
The balance check allocated a fresh slice for every selected row, purely to hold that row's message while it was looked up in the tally. On a 200,000 row bus carrying only 16 distinct messages that came to 200,043 allocations, every one of them avoidable: measured at exactly 8 bytes each for koalabear and 64 for bls12_377, which is one message apiece. Read each row into a buffer reused across rows instead. That is only safe if the tally never retains the buffer, so add hash.Map.Update, which retains a key only where it is genuinely new and then only a clone of it. Update also folds the lookup and the store into one visit, so a row now hashes its message once rather than twice. Measured over 200,000 rows: duplicated koalabear 10.3ms, 200043 allocs -> 4.20ms, 61 allocs duplicated bls12_377 18.1ms, 200043 allocs -> 8.97ms, 61 allocs alldistinct koalabear 26.2ms, 400558 allocs -> 26.7ms, 300560 allocs alldistinct bls12_377 30.9ms, 400558 allocs -> 29.6ms, 300560 allocs So a 2 to 2.45x win where messages repeat, and time-neutral where every message is distinct -- there a copy has to be made regardless. Of the allocations still left in that case only a third are those copies; the rest are two slices per bucket, which is structural to hash.Map. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Olivier Bégassat <olivier.begassat.cours@gmail.com>
Three points raised by Copilot on the merged PR, plus a missing fixture. NewConstraint took the expected port width from the sends alone, so a bus with receives but no sends left width at zero and then panicked on its first receive port -- reporting "inconsistent number of receive registers" when nothing was inconsistent. Take the width from whichever side has ports. Note this is defensive only: translateBus rejects a bus missing either direction before NewBusConstraint is reached, and Consistent checks it again. checkWithField counted sharded tests as the number of *lines* in the fixture. ReadShardedTracesFile returns one entry per line, leaving comments and blanks nil, so a fixture of nothing but comments would satisfy the "missing any tests" sanity check whilst checking nothing. Count the non-nil entries instead. Add bus_invalid_08, covering "bus has receives but no sends". bus_invalid_04 covered only the mirror case, leaving that error message untested. The remaining point -- that an indented ";;" comment is handed to the JSON parser rather than skipped -- is recorded in the findings notes instead. It affects all three fixture readers rather than the sharded one alone, nothing in testdata currently triggers it, and it has nothing to do with bus performance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Olivier Bégassat <olivier.begassat.cours@gmail.com>
The benchmarks did their job: they killed the idea of pre-sizing the tally (measurably 15-18% slower) and quantified the win from dropping the per-row allocation. Keeping them costs more than it returns. Nothing runs them: corset-bench and zkc-bench-test both use -run, so they select tests named Bench rather than Go benchmarks, and no target anywhere passes -bench. They would still be compiled by every go test ./..., making them a maintenance dependency on the trace and field APIs for no test value, and nothing would notice them rotting. They would rot soon, too. Handing the prover a per-shard bus digest -- where balance is the per-shard accumulators multiplying to one -- wants the tally keyed per shard and per side, which is neither axis the current global signed tally has. A benchmark pinned to today's shape would not survive that. The measurements are recorded in the PR description and in the findings notes, along with a pointer to this commit, since a squash merge will not leave it in main's history. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Olivier Bégassat <olivier.begassat.cours@gmail.com>
|
Condensed review comment:
|
|
I am going to close this because it conflicts with #2186, and I don't want this to merge before that one. I took the main pieces from this branch related to memory optimisation and the |
Pull request was closed
Follow-up to #2181. No change to which traces are accepted or rejected — same
verdicts, less work.
Measured, versus
main200,000 selected bus rows, medians of 3, same machine back to back.
alldistinct= every message distinct (the shape RAM consistency will have);duplicated= 16 distinct messages over all rows.mainAllocations: 400,556 → 300,560 (alldistinct), 200,041 → 61 (duplicated).
Spread on the alldistinct rows is ~15%, so treat those as approximate.
Measured with a throwaway benchmark, added and then reverted here (commit
6d9eae25if anyone wants it). Not kept: nothing in the repo runs Go benchmarks—
corset-benchandzkc-bench-testuse-run, selecting tests named Bench —so it would never execute, yet would still be compiled by every
go test ./....What changed
bus failures were then discarded — including two wasted
count()rescans perbus, since a lone shard rarely balances. Filters the constraints instead, via
getBusAndOtherConstraintsand the newschema.AcceptsSubset.the new
hash.Map.Updateretains a key just when it is new, and then clonesit.
Updatealso folds lookup and store into one visit, halving the hashing.This is @DavePearce's review comment on feat: lisp syntax and sharded trace checking for send and receive bus primitives #2181.
portColumns).NetTally(no callers); renamed fixture modulesalpha/betatosndr/rcvr;checkShardGrouptocheckShards, since a bus balances overthe complete set of shards, never a subset.
Review comments from #2181
NewConstrainttook its width from the sends alone, so receives-with-no-sendspanicked with a misleading message. Now taken from whichever side has ports.
Defensive only:
translateBusrejects such a bus first, andConsistentchecks it again.
"missing any tests" guard whilst checking nothing. Counts trace sets now.
bus_invalid_08, covering "receives but no sends" —bus_invalid_04covered only the mirror case, leaving that message untested.
;;comment reaches the JSON parser instead ofbeing skipped. It affects all three fixture readers rather than the sharded one
alone, nothing in
testdatatriggers it, and it is unrelated to busperformance. Noted for a separate issue.
Ideas measured and rejected
15-18% slower and used 11x the memory.
uint64fast path for messages narrow enough to pack into one word. Realbus messages are (address, value, timestamp) at roughly 64/64/48 bits, and
ports are built from limbs, so a RAM entry is ~11 registers on a 16-bit
register field. It would only ever fire on toy fixtures.
Stopping here deliberately
The ~200k allocations still left in the
alldistinctcase are structural tohash.Map, which gives every entry two slices. Replacing that — e.g. a Go mapkeyed on the message bytes — would cut them, but the tally is likely to be
restructured anyway: a per-shard bus digest for the prover wants it keyed per
shard and per side, neither of which the current global signed tally has.
Further work on its internals has a short shelf life until that is settled.
🤖 Generated with Claude Code