feat(stf): zero-alloc processSyncAggregate - #269
Conversation
separate `intersectValues` into: - `intersectValuesAlloc`: the original `intersectValues`. - `intersectValues`: a new function that takes a `out: [length]T` as a param. The maximum size of a `BitVector` is 512, which is negligible on the stack. We introduce an alloc-free version as a minor optimization.
We also alloc 512 * 96 = 48KiB worth of pubkeys previously on the heap, which we now switch to use arrays since that amount seems reasonable.
spiral-ladder
changed the title
processSyncAggregate
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 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 refactors the BitVector.intersectValues function by introducing a new non-allocating version that takes an output buffer, while renaming the original allocating version to intersectValuesAlloc. This change is propagated to the processSyncAggregate function and its callers, including benchmarks and tests, to eliminate dynamic memory allocations by utilizing stack-allocated buffers for participant indices and public keys. Review comments suggest adding null pointer assertions to the new intersectValues function's arguments, changing usize variables to explicitly-sized types like u32 within the same function, and minimizing the scope of participant_buf in getSyncCommitteeSignatureSet by declaring it within its else block, all to align with the project's style guide.
| pub fn intersectValues( | ||
| self: *const @This(), | ||
| comptime T: type, | ||
| values: *const [length]T, | ||
| out: *[length]T, | ||
| ) []T { |
There was a problem hiding this comment.
According to the style guide (rule 51), all function arguments should be asserted. This function is missing checks for its pointer arguments (self, values, out).
Please add assertions at the beginning of the function body to ensure these pointers are not null. This improves safety by catching potential null pointer dereferences early.
References
- Assert all function arguments and return values, pre/postconditions and invariants. A function must not operate blindly on data it has not checked. The purpose of a function is to increase the probability that a program is correct. Assertions within a function are part of how functions serve this purpose. The assertion density of the code must average a minimum of two assertions per function. (link)
| var i: usize = 0; | ||
| for (0..byte_len) |i_byte| { | ||
| var b = self.data[i_byte]; | ||
| // Kernighan's algorithm to count the set bits instead of going through 0..8 for every byte | ||
| while (b != 0) { | ||
| const lsb: usize = @as(u8, @ctz(b)); // Get the index of least significant bit | ||
| const bit_index = i_byte * 8 + lsb; | ||
| out[i] = values[bit_index]; | ||
| i += 1; | ||
| // The `b - 1` flips the bits starting from `lsb` index | ||
| // And `&` will reset the last bit at `lsb` index | ||
| b &= b - 1; | ||
| } |
There was a problem hiding this comment.
The style guide (rule 44) recommends avoiding usize in favor of explicitly-sized integer types like u32 to improve portability and explicitness. The variables i, lsb, and bit_index are currently usize.
Please consider changing them to a fixed-size type like u32.
var i: u32 = 0;
for (0..byte_len) |i_byte| {
var b = self.data[i_byte];
// Kernighan's algorithm to count the set bits instead of going through 0..8 for every byte
while (b != 0) {
const lsb: u32 = @ctz(b); // Get the index of least significant bit
const bit_index: u32 = @as(u32, i_byte) * 8 + lsb;
out[i] = values[bit_index];
i += 1;
// The `b - 1` flips the bits starting from `lsb` index
// And `&` will reset the last bit at `lsb` index
b &= b - 1;
}
References
- Use explicitly-sized types like
u32for everything, avoid architecture-specificusize. (link)
| var participant_buf: [preset.SYNC_COMMITTEE_SIZE]ValidatorIndex = undefined; | ||
| const participant_indices_ = if (participant_indices) |pi| pi else blk: { | ||
| const committee_indices = @as(*const [preset.SYNC_COMMITTEE_SIZE]u64, @ptrCast(epoch_cache.current_sync_committee_indexed.get().getValidatorIndices())); | ||
| break :blk (try sync_aggregate.sync_committee_bits.intersectValues(ValidatorIndex, allocator, committee_indices)).items; | ||
| break :blk sync_aggregate.sync_committee_bits.intersectValues(ValidatorIndex, committee_indices, &participant_buf); | ||
| }; |
There was a problem hiding this comment.
To minimize the scope of participant_buf and adhere to style guide rules 104 and 356, it should be declared inside the else block where it is used.
const participant_indices_ = if (participant_indices) |pi| pi else blk: {
const committee_indices = @as(*const [preset.SYNC_COMMITTEE_SIZE]u64, @ptrCast(epoch_cache.current_sync_committee_indexed.get().getValidatorIndices()));
var participant_buf: [preset.SYNC_COMMITTEE_SIZE]ValidatorIndex = undefined;
break :blk sync_aggregate.sync_committee_bits.intersectValues(ValidatorIndex, committee_indices, &participant_buf);
};
References
- Declare variables at the smallest possible scope, and minimize the number of variables in scope, to reduce the probability that variables are misused. (link)
- Calculate or check variables close to where/when they are used. Don't introduce variables before they are needed. Don't leave them around where they are not. (link)
lodekeeper-z
left a comment
lodekeeper-z
left a comment
There was a problem hiding this comment.
Clean optimization — stack-allocated buffers for sync aggregate processing. The new intersectValues correctly mirrors the existing Kernighan's algorithm logic.
One minor observation: getSyncCommitteeSignatureSet still heap-allocates pubkeys (line 161) — could be stack-allocated too for consistency, but fine as a follow-up.
48KB total stack usage (4KB indices + ~24KB pubkeys in processSyncAggregate, + 4KB in getSyncCommitteeSignatureSet) is well within safe limits.
lodekeeper-z
left a comment
lodekeeper-z
left a comment
There was a problem hiding this comment.
CI is failing — there's a second processSyncAggregate call site in bench/state_transition/process_block.zig:421 that still passes allocator. Needs the same fix as the one at line 218.
bench/state_transition/process_block.zig:421:33: error: expected 6 argument(s), found 7
Also: getSyncCommitteeSignatureSet still takes allocator as a param (line 117 in the diff) but no longer uses it for intersectValues — worth removing it from that function's signature too for consistency? Though it might still be used for other things in that function, haven't checked.
|
CI is failing because |
lodekeeper-z
left a comment
lodekeeper-z
left a comment
There was a problem hiding this comment.
CI is failing — there's a second processSyncAggregate call site in bench/state_transition/process_block.zig:421 that still passes allocator. The one at line 215 was updated but this one was missed.
- state_transition.processSyncAggregate(
- fork,
- allocator,
- cloned.config,
+ state_transition.processSyncAggregate(
+ fork,
+ cloned.config,Otherwise the PR looks good — nice zero-alloc optimization 👍
lodekeeper-z
left a comment
lodekeeper-z
left a comment
There was a problem hiding this comment.
Clean change. The zero-alloc approach with stack buffers is correct — SYNC_COMMITTEE_SIZE is compile-time known and fits comfortably on the stack.
Nice side-effect: this also fixes a memory leak in getSyncCommitteeSignatureSet where the old intersectValues returned an ArrayList whose .items was extracted but the list itself was never deinit'd.
Kernighan's bit iteration in the new intersectValues matches the existing allocating version — consistent behavior. 👍
lodekeeper-z
left a comment
lodekeeper-z
left a comment
There was a problem hiding this comment.
Clean PR. Stack-allocated buffers for sync committee processing — eliminates heap allocations for both participant_indices and pubkeys.
Nice side-effect: fixes a memory leak in getSyncCommitteeSignatureSet where the old code did (try ...intersectValues(...)).items which took the backing slice but never deinitialized the ArrayList.
Kernighan's bit trick in the new intersectValues is a good touch — skips zero bytes efficiently.
LGTM ✅
|
CI is failing on all 3 platforms — the bench file |
|
Hey @spiral-ladder — looks like this needs a rebase against |
|
@spiral-ladder is this still relevant with all the other great work youve done on blst? |
|
@matthewkeil i think it is but low prio, since it's just an optimization for state transition one thing to discuss here is if its worth the change in the ssz api for this |
3 participants
Introduces a zero-alloc
processSyncAggregate.Done by:
separating
intersectValuesinBitVectorinto:intersectValuesAlloc: the originalintersectValues.intersectValues: a new function that takes aout: [length]Tas a param.The maximum size of the
BitVectoris 512, which is negligible on thestack. We introduce an alloc-free version as a minor optimization.
Then since we know the pubkeys are going to be 512 * 96 = 48KiB worth of allocations,
we can skip heap allocations and simply use an array for
participant_buf.