Skip to content

feat(bench): add corpus-gen progress logging and speed up sampling - #3954

Merged
rodrodros merged 1 commit into
mainfrom
dat/bench-full-flow
Aug 19, 2026
Merged

feat(bench): add corpus-gen progress logging and speed up sampling#3954
rodrodros merged 1 commit into
mainfrom
dat/bench-full-flow

Conversation

@infrmtcs-agent

Copy link
Copy Markdown
Contributor

Summary

  • Add progress logging to the corpus-gen benchmark tool
  • Speed up sampling in corpus-gen

@infrmtcs
infrmtcs requested a review from rodrodros August 18, 2026 12:26
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @infrmtcs-agent[bot]'s task in 5m 10s —— View job


Review: corpus-gen progress logging + faster sampling

  • Fetch PR diff vs origin/main
  • Read repo guidelines (CLAUDE.md, .golangci.yaml)
  • Review corpus-gen changes (progress logging, resample, sierraCache)
  • Check deleted corpus JSON files for script breakage
  • Build / vet the package
  • Post findings (4 inline comments)

No blockers. The refactor is a genuine improvement: extracting resample removes the "whole-sampler restart" behaviour in storageProofSampler (a single bad draw no longer discards the already-built params), errors now carry which sub-draw failed, and the progress goroutine's lifecycle is correct — one close(quit) + wg.Wait(), defer'd before the pool starts, so it can't leak on the error path, and it can't interleave with runCorpus's final stderr write.

Verified locally: go build ./bench/... and go vet ./bench/rpc/cmd/corpus-gen/ are clean. golang.org/x/sync is already a direct dependency (go.mod:41), so the singleflight import adds nothing new. The ignored fmt.Fprintf return in the progress goroutine is covered by the std-error-handling errcheck preset — not a lint failure.

Important

  • get_storage_proof.go:49-53 — dropping the Sierra filter turns these into non-membership proofs, which terminate at the divergence node and omit the leaf. That understates class-proof cost, and by an amount that drifts with classes-trie density rather than staying stable across runs. The getClass round trip it avoids is already eliminated by the new sierraCache, so it's worth confirming the cache alone wasn't sufficient here. Details inline.

Nits

  • sampler.go:82-94 — nested resample gives each level an independent 100-attempt budget with no shared cap; getStorageProof --num-keys 100 can reach ~10k getStateUpdate fetches for one entry. Related: the bigger speedup left on the table is caching state updates, not Sierra verdicts — every resample still pays a full getStateUpdate, and those are the heaviest responses this tool downloads.
  • sierra_cache.go:31-45 — the flight closure binds all waiters to the winning caller's ctx; benign here since conc gives every worker the same derived context, but worth being deliberate about. client is also invariant per command and could live on the cache instead of in samplerInput.
  • main.go:254-275completed advances per entry, so --batch 100 shows 0/1000 for a long time; a fixed 2s interval is also noisy across gen-all.sh's 17 corpora.
  • Deleted corpus/with_state_update.json / without_state_update.json — these were added two commits ago in feat(bench): add gen-all and run-all scripts for the full RPC benchmark flow #3950 and are superseded by corpus/all.json. I grepped README.md, gen-all.sh, run-all.sh, run.js, throughput.js and the Makefile: nothing references them, so the deletion is safe. It just isn't mentioned in the PR description or commit message — worth a line there, since a reader of the log would otherwise assume the progress-logging change removed them for a reason.
  • Testsresample and sierraCache are the two pieces here with real logic and no I/O coupling (resample is fully pure; sierraCache needs only a stub classAt). The package has no tests today, so this isn't a regression, but a table-driven test for resample (success first try / errResample then success / non-errResample passthrough / exhaustion message) and a concurrent isSierra test asserting one classAt call per hash under -race would lock in exactly the two behaviours this PR depends on.
    · branch dat/bench-full-flow

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 83 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.50%. Comparing base (c5a1917) to head (9405941).

Files with missing lines Patch % Lines
bench/rpc/cmd/corpus-gen/main.go 0.00% 26 Missing ⚠️
bench/rpc/cmd/corpus-gen/sierra_cache.go 0.00% 24 Missing ⚠️
bench/rpc/cmd/corpus-gen/get_storage_proof.go 0.00% 16 Missing ⚠️
bench/rpc/cmd/corpus-gen/sampler.go 0.00% 15 Missing ⚠️
bench/rpc/cmd/corpus-gen/class_hash.go 0.00% 2 Missing ⚠️

❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3954      +/-   ##
==========================================
- Coverage   74.75%   74.50%   -0.25%     
==========================================
  Files         461      462       +1     
  Lines       40928    40989      +61     
==========================================
- Hits        30594    30538      -56     
- Misses       8230     8353     +123     
+ Partials     2104     2098       -6     
Flag Coverage Δ
jsonv2 77.42% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@infrmtcs-agent
infrmtcs-agent Bot deployed to Development August 18, 2026 12:31 Active
Comment thread bench/rpc/cmd/corpus-gen/get_storage_proof.go
Comment thread bench/rpc/cmd/corpus-gen/sampler.go
Comment thread bench/rpc/cmd/corpus-gen/sierra_cache.go
Comment thread bench/rpc/cmd/corpus-gen/main.go
@rodrodros
rodrodros merged commit d5d68c0 into main Aug 19, 2026
27 of 30 checks passed
@rodrodros
rodrodros deleted the dat/bench-full-flow branch August 19, 2026 14:36
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.

2 participants