chore: move state clones out of benchmark run functions#324
chore: move state clones out of benchmark run functions#324markolazic01 wants to merge 6 commits intoChainSafe:mainfrom
Conversation
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 optimizes the benchmark suite by decoupling state management from the timed execution logic. By moving the cloning of 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
|
There was a problem hiding this comment.
Code Review
This pull request refactors the state transition benchmarks in process_block.zig and process_epoch.zig to use global state instances managed through beforeEach and afterEach hooks, centralizing state cloning and deinitialization. Feedback focuses on several style guide violations, specifically regarding the requirement for a minimum assertion density of two per function, the need to pass explicit options to library functions, adherence to the 100-column line length limit, and correct 4-space indentation.
| fn beforeEach() void { | ||
| g_cached_state_instance = g_cached_state.clone(g_allocator, .{}) catch unreachable; | ||
| } |
There was a problem hiding this comment.
This function violates the repository's style guide regarding assertion density and explicit options.
- Assertion Density: The style guide (Line 54) requires an average of at least two assertions per function. Assertions are critical for detecting programmer errors and documenting invariants.
- Explicit Options: The style guide (Line 170) requires explicitly passing options to library functions instead of relying on defaults. The
clonefunction should be called with an explicit options struct.
fn beforeEach() void {
std.debug.assert(g_allocator.ptr != null);
g_cached_state_instance = g_cached_state.clone(g_allocator, .{ .transfer_cache = true }) catch unreachable;
std.debug.assert(g_cached_state_instance.state.ptr != null);
}
There was a problem hiding this comment.
Seems not applicable for this particular case, same goes for the other comments mentioning hooks.
| fn afterEach() void { | ||
| g_cached_state_instance.deinit(); | ||
| g_allocator.destroy(g_cached_state_instance); | ||
| } |
There was a problem hiding this comment.
This function lacks assertions. According to the style guide (Line 54), the assertion density must average at least two assertions per function. Consider asserting that the instance being freed is valid.
fn afterEach() void {
std.debug.assert(g_cached_state_instance != undefined);
g_cached_state_instance.deinit();
g_allocator.destroy(g_cached_state_instance);
}
References
- The assertion density of the code must average a minimum of two assertions per function. (link)
| allocator.destroy(cloned); | ||
| } | ||
|
|
||
| const external_data = BlockExternalData{ .execution_payload_status = .valid, .data_availability_status = .available }; |
There was a problem hiding this comment.
This line exceeds the 100-column limit defined in the style guide (Line 400). Hard limiting line lengths ensures better readability and avoids horizontal scrolling.
const external_data = BlockExternalData{
.execution_payload_status = .valid,
.data_availability_status = .available,
};
References
- Hard limit all line lengths, without exception, to at most 100 columns. (link)
There was a problem hiding this comment.
This also seems unrelated as the repo isn't enforcing any styling requirements atm, and this line is unaffected by the present changes.
| fn beforeEach() void { | ||
| g_cached_state_instance = g_cached_state.clone(g_allocator, .{}) catch unreachable; | ||
| } |
There was a problem hiding this comment.
This function violates the style guide requirements for assertion density (Line 54) and explicit options (Line 170).
fn beforeEach() void {
std.debug.assert(g_allocator.ptr != null);
g_cached_state_instance = g_cached_state.clone(g_allocator, .{ .transfer_cache = true }) catch unreachable;
std.debug.assert(g_cached_state_instance.state.ptr != null);
}
|
Resolved conflicts in the latest commit. |
|
All the checks should pass now |
bench: move state clones out of benchmark run functions
Closes #164
Problem
Benchmark
runfunctions were cloning and freeingCachedBeaconStateon every iteration. This clone/deinit cost was included in the timed measurement, skewing results forprocessBlock,processEpoch, and their individual sub-step benchmarks. This is visible through therunImplinsidezbench. This change should also increase readability of the affected files.Solution
Moved state cloning into zbench's
before_each/after_eachlifecycle hooks using module-level variables:beforeEachclones the cached state before each timed iterationafterEachfrees the clone after the iteration completesrunfunctions now operate directly on the pre-cloned instanceAlso removed the now-unused
cached_statefield from all benchmark structs and cleaned up the correspondingaddParamcalls.Applied to
process_block.zigandprocess_epoch.zig.AI disclosure: Claude was consulted for reviewing the approach and drafting this description. All code changes were authored manually.