perf(epoch): replace AutoHashMap with array lookup in reward/penalty caches#286
perf(epoch): replace AutoHashMap with array lookup in reward/penalty caches#286lodekeeper-z wants to merge 4 commits intoChainSafe:mainfrom
Conversation
…caches Replace heap-allocated AutoHashMap keyed by effective_balance_increment with stack-allocated fixed-size arrays in three epoch processing functions: - getRewardsAndPenaltiesAltair - getAttestationDeltas - processSlashings effective_balance_increment is bounded (max 32 pre-Electra, max 2048 post-Electra), so a fixed array provides O(1) lookup with zero allocation overhead. This eliminates hashing and heap allocation in the hot per-validator loops during epoch transitions. Also removes now-unused allocator parameters from these functions, simplifying call signatures. Refs: ChainSafe#243 🤖 Generated with AI assistance
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 performance of epoch processing by replacing dynamic hash map lookups with direct array indexing. By leveraging the bounded nature of effective balance increments, the changes reduce memory pressure and improve cache locality in hot validator loops, resulting in more efficient state transitions. 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 optimizes epoch state transitions by replacing dynamic std.AutoHashMap caches with fixed-size arrays in getAttestationDeltas, getRewardsAndPenaltiesAltair, and processSlashings. These changes eliminate the need for an Allocator in several functions, reducing overhead from hashing and memory allocation in performance-critical validator loops. The cache sizes are determined at compile-time based on the Electra preset. I have no feedback to provide as the changes align with the performance goals of the repository.
processSlashings signature changed in this PR to remove the allocator parameter (no longer needed after switching from AutoHashMap to array lookup). Update the two call sites in bench/state_transition/process_epoch.zig. 🤖 Generated with AI assistance
…enalties signatures processSlashings no longer takes an allocator param. getRewardsAndPenalties no longer takes an allocator param. 🤖 Generated with AI assistance
| // effectiveBalance is a multiple of EFFECTIVE_BALANCE_INCREMENT and bounded by max effective balance, | ||
| // so effective_balance_increment has a small range (0..max_increment). Use a fixed array for O(1) lookup | ||
| // instead of a HashMap — avoids allocation and hashing overhead in the hot validator loop. | ||
| const max_increment = comptime preset.MAX_EFFECTIVE_BALANCE_ELECTRA / preset.EFFECTIVE_BALANCE_INCREMENT + 1; |
There was a problem hiding this comment.
getAttestationDeltas() is supposed for phase0 so need to use MAX_EFFECTIVE_BALANCE instead
also would be great to add a comment for this function stating phase0
There was a problem hiding this comment.
Fixed in 8f728c3. Changed to use MAX_EFFECTIVE_BALANCE (32 increments) since this function is phase0-only. Added comment noting it's phase0.
There was a problem hiding this comment.
Fixed in 8f728c3. Changed to use MAX_EFFECTIVE_BALANCE (32 increments) since this function is phase0-only. Added comment noting it's phase0.
| // so effective_balance_increment has a small range (0..max_increment). Use a fixed array for O(1) lookup | ||
| // instead of a HashMap — avoids allocation and hashing overhead in the hot validator loop. | ||
| const max_increment = comptime preset.MAX_EFFECTIVE_BALANCE_ELECTRA / EFFECTIVE_BALANCE_INCREMENT + 1; | ||
| var reward_penalty_item_cache: [max_increment]?RewardPenaltyItem = .{null} ** max_increment; |
There was a problem hiding this comment.
for pre-electra, use MAX_EFFECTIVE_BALANCE to save memory
There was a problem hiding this comment.
Fixed in 8f728c3. Now uses comptime fork selection: MAX_EFFECTIVE_BALANCE (33 slots) for pre-Electra forks, MAX_EFFECTIVE_BALANCE_ELECTRA (2049 slots) for Electra+. Same for processSlashings.
| @@ -74,7 +73,7 @@ pub fn getRewardsAndPenaltiesAltair( | |||
|
|
|||
| const effective_balance_increment = effective_balance_increments[i]; | |||
There was a problem hiding this comment.
it's a little bit unsafe because effective_balance_increment is modeled as u16 pub const EffectiveBalanceIncrements = std.ArrayList(u16);
maybe switch it to u11 to match 2048 as in electra?
There was a problem hiding this comment.
Valid concern. The u16 type allows values up to 65535 but the array is at most 2049. With the fork-aware sizing now in place, the arrays are even smaller (33 for phase0/pre-Electra).
Changing EffectiveBalanceIncrements from ArrayList(u16) to a narrower type (u12 would cover 0..2048) is a broader change touching epoch_cache, epoch_transition_cache, and all consumers. Worth doing as a separate PR — want me to open one?
- getAttestationDeltas (phase0 only): use MAX_EFFECTIVE_BALANCE (32 increments) - getRewardsAndPenaltiesAltair: use comptime fork to select max (32 pre-Electra, 2048 post) - processSlashings: same fork-aware sizing Addresses review feedback from twoeths on PR ChainSafe#286. 🤖 Generated with AI assistance
Summary
Replace heap-allocated
AutoHashMapkeyed byeffective_balance_incrementwith stack-allocated fixed-size arrays in three epoch processing functions:getRewardsAndPenaltiesAltairgetAttestationDeltasprocessSlashingsMotivation
effective_balance_incrementis bounded: max 32 pre-Electra, max 2048 post-Electra. The existingAutoHashMapperforms unnecessary heap allocation and hashing in the hot per-validator loops during epoch transitions (~1.1M validators on mainnet).A fixed-size
[max_increment]?Tarray provides:Changes
std.AutoHashMapwith[max_increment]?RewardPenaltyItem/[max_increment]?u64arraysallocatorparameters fromgetRewardsAndPenaltiesAltair,getAttestationDeltas,processSlashings, andgetRewardsAndPenaltiesTesting
All
test:state_transitiontests pass.Refs: #243
🤖 Generated with AI assistance