fix(ci): budget the root-rollup VK test to its measured cost, drop a redundant circuit copy - #25127
Draft
AztecBot wants to merge 2 commits into
Draft
fix(ci): budget the root-rollup VK test to its measured cost, drop a redundant circuit copy#25127AztecBot wants to merge 2 commits into
AztecBot wants to merge 2 commits into
Conversation
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.
The flake
barretenberg/cpp/scripts/run_test.sh dsl_tests HonkRecursionConstraintTestWithoutPredicate/2.GenerateVKFromConstraintsfails ~12% of the time onnext(history). Over the last 75 tracked runs:code: 124Every failure is
timeoutfiring at the defaultTIMEOUT=600s, and the failure runtimes cluster in a 12-second band right at that deadline.It is not a hang, and not a cleanup issue
[ OK ]. They areSIGTERM'd mid-computation, 10–90s short of finishing — e.g. b36f4b69785190ee dies 11s afterpopulating trace...in the final block.[ OK ](01dc57c4852bab07) reports(598699 ms)and is killed 1.3s later. It consumed 598.7s of its 600s budget; there is no post-PASSEDstall, just no clock left for teardown.timeoutline at all.The ~5x spread comes from the runner:
ci3/parallelizeruns-j$((nproc / 2))= 32 concurrent commands, andci3/exec_testruns each undertaskset -c 0-63— the whole CPU set.CPUS=8only setsHARDWARE_CONCURRENCYinside bb; it does not confine the test to 8 cores. Separately, GNU parallel's--memsuspendSIGSTOPs jobs under memory pressure whiletimeoutkeeps counting, and at ~19.8GiB this is the largest-footprint test in the suite.Where the time actually goes
Profiled on an idle 192-core box with
HARDWARE_CONCURRENCY=8(matching CI), by timing each phase oftest_vk_independence. Baseline total 182.5s:generate_constraints(inner circuits + proofs)create_circuit(with witness)get_num_finalized_gates_inefficientProverInstanceVerificationKey(28 MSMs over 8.4M points)CircuitChecker::checkcreate_circuit(no witness)ProverInstanceVerificationKeyCircuitChecker::checkbreaks down further into:prepare_circuit(a second full copy of the 6.35M-gate builder + finalize) 9.1s, lookup hash-table build 15.3s, and the serial per-row relation loop 62.1s over 6.35M rows across 9 blocks (~10µs/row).What this PR changes
1.
TIMEOUT=20m,MEM=16g→20gon theHonkRecursionConstraintTestWithoutPredicate/2.*prefix, matching whatChonkPinnedIvcInputsTest.AllPinnedFlowsalready does. 20m is 2x headroom over the worst observed contended runtime; the default 600s left none over a 486s observed maximum.MEMis documentation for non-ISOLATEtests (onlydocker_isolateenforces it), but 16g understated the real ~19.8GiB peak and it should be true.2. Drop
get_num_finalized_gates_inefficient()fromtest_vk_independence. That helper deep-copies the entire builder, finalizes the copy, counts gates, and throws the copy away — its doc comment says as much. Constructing theProverInstanceon the next line already finalizes the real builder, soget_num_finalized_gates()returns the identical value for free. Measured: 192.5s → 182.6s on the root rollup, and the transient copy (~2.7GiB) is gone. Everyacir_formatconstraint test that callstest_vk_independencebenefits.The returned
num_gatesis discarded by all 14 call sites, so no behavior depends on it;finalize_circuit()takes no arguments, so both paths finalize identically.Things measured and rejected
batch_commitinstead of the per-polynomialcommitloop inNativeVerificationKey_(would target the 41.5s of VK commitments, 23%): implemented and measured — slower, 221s vs 192s baseline. Batching 28 MSMs over 8.4M points costs more in memory traffic than it saves at 8 threads. Not pursued.CPUS8 → 16: 182.6s → 159.2s (-13%); 8 → 32 regresses to 188.0s. Left alone deliberately —CPUSdoes not confine the test to a cpuset, so extra threads are taken from the 31 other jobs sharing the box. Worth revisiting only alongside real cpuset isolation.CircuitChecker::check: tempting, sincetest_tamperingnormally covers it viaInvalidWitness::Target::None. But forIsRootRollupthe target list is deliberately trimmed to{ VKHash }("Only one for Root because it is very heavy"), soTarget::Nonenever runs for this circuit.GenerateVKFromConstraintsis the only place the valid root-rollup circuit is checked for satisfiability. Removing it would silently drop that coverage, so it stays.The remaining lever
CircuitChecker::checkis 47% of this test and is serial by construction —check_blockthreadsTagCheckData/MemoryCheckDataaccumulators through a row-by-row loop. Parallelizing it (or giving it a fast path that skipsprepare_circuit's copy when the builder is already finalized, worth 9s here) would speed up every bb circuit test, not just this one. That is real work on the reference checker with correctness stakes, so it is not bundled here.Validation
bash -n barretenberg/cpp/bootstrap.shpasses; the new prefix parses correctly throughci3/source_test_params(CPUS=8 MEM=20g TIMEOUT=20m), andtest_cmdis unchanged, so the existing history list stays continuous.dsl_tests --gtest_filter='*GenerateVKFromConstraints*'(excluding the two heaviest recursion params): 178/178 pass.HonkRecursionConstraintTestWithoutPredicate/2.GenerateVKFromConstraintspasses locally; 192.5s baseline → 182.6s after, peak RSS 19.83GiB.Created by claudebox · group:
slackbot· requested by ludamad (@ludamad) · Slack thread