benchmark: add the Gorums workloads and the benchmark binary - #336
Open
meling wants to merge 4 commits into
Open
benchmark: add the Gorums workloads and the benchmark binary#336meling wants to merge 4 commits into
meling wants to merge 4 commits into
Conversation
Contributor
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go |  |
Aug 12, 2026 12:38p.m. | Review ↗ |
| Shell | |
Aug 12, 2026 12:38p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces Gorums benchmark workloads and a new benchkit/cmd/benchmark node binary, expanding the benchkit module with a symmetric peer-to-peer harness and build integration so benchmarks can be run locally, coordinated, or fully distributed.
Changes:
- Add the
benchkit/benchmarkpackage implementing benchmark descriptors, symmetric targets, and workload servers (plus extensive tests). - Add
benchkit/cmd/benchmarkbinary with flags/target setup, reporting, and distributed teardown coordination. - Update build tooling/docs to generate benchmark protos and build the benchmark binary.
Reviewed changes
Copilot reviewed 10 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds benchmark proto generation rules and a benchmark build target; wires it into all/genproto. |
| benchkit/cmd/benchmark/main.go | New benchmark runner binary: flags, target setup, running benchmarks, reporting, and distributed teardown. |
| benchkit/cmd/benchmark/main_test.go | Unit tests for flag normalization and derived option calculations (quorum size, buffers, stream mode). |
| benchkit/benchmark/target.go | Target setup for local/coordinator/distributed modes, including readiness and dedup sequencing. |
| benchkit/benchmark/target_test.go | Comprehensive tests for target setup, readiness probing, dedup behavior, exit grace, and failure diagnostics. |
| benchkit/benchmark/symmetric.go | Symmetric peer harness, readiness probing, exit signaling/grace logic, and symmetric benchmark runners. |
| benchkit/benchmark/server.go | Workload RPC server implementation + shared control-plane registration helper. |
| benchkit/benchmark/benchmark.go | Descriptor-table-driven benchmark registration and runners (including AsyncMulticast bounding). |
| benchkit/benchmark/benchmark_test.go | Extensive behavioral tests for benchmark selection/routing and runner semantics. |
| benchkit/benchmark/benchmark.pb.go | Generated protobuf definitions for benchmark workload messages/service. |
| benchkit/benchmark/benchmark_gorums.pb.go | Generated Gorums bindings for the benchmark workload service. |
| AGENTS.md | Documents the new benchmark and binary directories. |
| .gitignore | Ignores the built benchmark binary artifact. |
Files not reviewed (2)
- benchkit/benchmark/benchmark.pb.go: Generated file
- benchkit/benchmark/benchmark_gorums.pb.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @echo "Regenerating all proto files (dev, benchkit, benchmark, internal/tests, examples)" | ||
| @$(MAKE) -B -s dev | ||
| @$(MAKE) -B -s $(benchkit_deps) | ||
| @$(MAKE) -B -s benchmark |
Comment on lines
+159
to
+160
| req := benchkit.DoneRequest_builder{SenderId: t.controls[i].SelfID()}.Build() | ||
| _ = benchkit.Done(out.Context(ctx), req).Send() |
Comment on lines
+128
to
+140
| func (f *flags) report(results []*benchkit.Result, opts benchkit.Options) { | ||
| benchkit.PrintResults(os.Stdout, results, opts, f.serverStats, f.Self) | ||
| if f.Output != "" { | ||
| label := f.label | ||
| if label == "" && f.Self != "" { | ||
| label = f.Self | ||
| } | ||
| checkf("Failed to write results: %v", benchkit.WriteLabeledReport(results, label, f.Output)) | ||
| } | ||
| if f.compare != "" { | ||
| checkf("Failed to compare results: %v", benchkit.CompareWithBaseline(f.compare, f.label, results, os.Stdout)) | ||
| } | ||
| } |
meling
force-pushed
the
benchmark/workloads
branch
from
August 12, 2026 11:42
94ac85d to
0b94e5f
Compare
The workloads that exercise Gorums through benchkit: a descriptor table that drives both the list and the runners, so a new workload is one entry rather than edits in three places, and a symmetric peer harness where every node is both client and server, which is what the dedup evaluation measures. AsyncMulticast is bounded by an explicit depth limit rather than left to run ahead unboundedly, and the default quorum size is a majority rather than the full configuration, so a single slow node does not define the measurement. cmd/benchmark is the node binary. It applies server options in local and distributed mode alike, which previously diverged, bounds the done signal so teardown cannot block on a peer that already exited, and reports one-way send errors instead of discarding them.
With -label unset in distributed mode, the written report fell back to -self for its label but the comparison still passed the empty -label, so the comparison output carried no experiment name even though one was available. Derive the label once and use it for both.
genproto invoked the phony benchmark target, which also links the benchmark binary, so a codegen-only request did a go build as a side effect. Depend on benchmark_deps instead; it is the generated-file list the benchmark target itself uses, and it already covers benchkit_deps.
SignalDone dropped the one-way send error with nothing to say it was deliberate. It stays deliberate, since a peer that has already finished and exited makes a failed send the expected outcome, but logging it separates that case from a run where every send failed and no peer was ever told.
meling
force-pushed
the
benchmark/workloads
branch
from
August 12, 2026 12:38
0b94e5f to
8d5e9ae
Compare
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.
Adds the Gorums workloads that exercise the library through
benchkit, and the node binary that runs them.A descriptor table drives both the workload list and the runners, so adding a workload is one entry rather than edits in three places. The symmetric peer harness makes every node both client and server, which is the topology the stream-deduplication evaluation measures.
Correctness details worth flagging:
AsyncMulticastis bounded by an explicit depth limit rather than left to run ahead unboundedly.This replaces the
benchmarkpackage andcmd/benchmarkretired earlier in the stack, now rewritten against the measurement harness instead of hand-rolled statistics.Verification:
go test ./... -count=1,go test ./benchkit/... -count=1,gofmt -l.