feat: add bench_ssz step#318
Conversation
While reviewing #291 I found it slightly inconvenient to run each bench_ssz_* step. This PR allows the user to run all the ssz benchmarks at once.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request simplifies the benchmarking workflow by introducing a consolidated build step. By aggregating multiple individual SSZ benchmark tasks into a single command, it improves developer efficiency and reduces the manual effort required to run performance tests. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new run:bench_ssz step to execute multiple SSZ benchmarks simultaneously. The feedback highlights that using addSystemCommand to invoke zig build recursively is an anti-pattern in Zig build scripts, as it bypasses the build graph's dependency management and parallel execution capabilities. Instead, the step should directly depend on the individual benchmark steps. Additionally, a line length violation of the 100-column limit was identified, and a more descriptive, non-redundant step description was suggested.
| const run_bench_ssz = b.addSystemCommand(&.{ "zig", "build", "run:bench_ssz_attestation", "run:bench_ssz_block", "run:bench_ssz_state" }); | ||
| const tls_run_bench_ssz = b.step("run:bench_ssz", "Run the bench_ssz run"); | ||
| tls_run_bench_ssz.dependOn(&run_bench_ssz.step); |
There was a problem hiding this comment.
Calling zig build via addSystemCommand is an anti-pattern in Zig build scripts. It spawns a sub-process, which is inefficient and breaks the build graph's ability to manage dependencies and parallel execution. Since the individual benchmark steps are already defined in this file, the run:bench_ssz step should directly depend on them.
Additionally, line 1032 exceeds the 100-column limit (Style Guide Repo line 400), and the description "Run the bench_ssz run" is redundant.
Note: As this file is marked as generated, you should ensure that the tool zbuild or its configuration in zbuild.zon is updated to produce the correct dependency-based logic instead of a shell command.
const tls_run_bench_ssz = b.step("run:bench_ssz", "Run all SSZ benchmarks");
tls_run_bench_ssz.dependOn(&run_exe_bench_ssz_attestation.step);
tls_run_bench_ssz.dependOn(&run_exe_bench_ssz_block.step);
tls_run_bench_ssz.dependOn(&run_exe_bench_ssz_state.step);
References
- Hard limit all line lengths, without exception, to at most 100 columns. (link)
1 participant
While reviewing #291 I found it slightly inconvenient to run each bench_ssz_* step. This PR allows the user to run all the ssz benchmarks at once.