feat: add StructContainerTreeView, model Validator as StructContainerType#247
feat: add StructContainerTreeView, model Validator as StructContainerType#247wemeetagain wants to merge 2 commits intomainfrom
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 introduces a significant performance enhancement for handling fixed-size containers within the SSZ (Simple Serialize) library. By implementing 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. Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 StructContainerTreeView, a significant performance optimization for fixed-size containers like Validator, by caching the value as a struct. The implementation is well-structured and includes a comprehensive set of tests. My review focuses on improving adherence to the repository's style guide and fixing a minor issue in the clone implementation. Specifically, I've suggested adding assertions for increased safety, correcting the behavior of clone to respect its options, and wrapping long lines to maintain code readability.
| pub fn clone(self: *Self, opts: CloneOpts) !*Self { | ||
| _ = opts; | ||
| try self.commit(); | ||
|
|
||
| try self.pool.ref(self.root); | ||
| errdefer self.pool.unref(self.root); | ||
|
|
||
| const ptr = try self.allocator.create(Self); | ||
| ptr.* = .{ | ||
| .allocator = self.allocator, | ||
| .pool = self.pool, | ||
| .root = self.root, | ||
| .value = self.value, | ||
| .changed = std.StaticBitSet(ST.chunk_count).initEmpty(), | ||
| }; | ||
| return ptr; | ||
| } |
There was a problem hiding this comment.
The clone function takes an opts: CloneOpts parameter but then ignores it. The transfer_cache option should be respected. When transfer_cache is false, the value should be re-decoded from the merkle tree root, similar to how it's done in init. The current implementation always transfers the cached value, effectively ignoring opts.transfer_cache = false.
pub fn clone(self: *Self, opts: CloneOpts) !*Self {
try self.commit();
try self.pool.ref(self.root);
errdefer self.pool.unref(self.root);
const ptr = try self.allocator.create(Self);
errdefer self.allocator.destroy(ptr);
ptr.allocator = self.allocator;
ptr.pool = self.pool;
ptr.root = self.root;
ptr.changed = std.StaticBitSet(ST.chunk_count).initEmpty();
if (opts.transfer_cache) {
ptr.value = self.value;
} else {
try ST.tree.toValue(self.root, self.pool, &ptr.value);
}
return ptr;
}
| pub fn init(allocator: Allocator, pool: *Node.Pool, root: Node.Id) !*Self { | ||
| try pool.ref(root); | ||
| errdefer pool.unref(root); | ||
|
|
||
| const ptr = try allocator.create(Self); | ||
| errdefer allocator.destroy(ptr); | ||
|
|
||
| try ST.tree.toValue(root, pool, &ptr.value); | ||
|
|
||
| ptr.allocator = allocator; | ||
| ptr.pool = pool; | ||
| ptr.root = root; | ||
| ptr.changed = std.StaticBitSet(ST.chunk_count).initEmpty(); | ||
| return ptr; | ||
| } |
There was a problem hiding this comment.
The style guide (lines 51-54) mandates asserting all function arguments, preconditions, and postconditions to improve safety. This function, and others in StructContainerTreeView, are missing assertions. For example, you could assert that pool is not null upon entry and that the returned pointer's fields are correctly initialized upon exit.
pub fn init(allocator: Allocator, pool: *Node.Pool, root: Node.Id) !*Self {
std.debug.assert(pool != null);
try pool.ref(root);
errdefer pool.unref(root);
const ptr = try allocator.create(Self);
errdefer allocator.destroy(ptr);
try ST.tree.toValue(root, pool, &ptr.value);
ptr.allocator = allocator;
ptr.pool = pool;
ptr.root = root;
ptr.changed = std.StaticBitSet(ST.chunk_count).initEmpty();
return ptr;
}
References
- Functions must assert all arguments, return values, preconditions, postconditions, and invariants. The assertion density should be at least two assertions per function. (link)
| inline for (ST.fields, 0..) |field, i| { | ||
| if (self.changed.isSet(i)) { | ||
| const ChildST = ST.getFieldType(field.name); | ||
| nodes[changed_idx] = try ChildST.tree.fromValue(self.pool, &@field(self.value, field.name)); |
There was a problem hiding this comment.
This line exceeds the 100-column limit specified in the style guide (line 400). Extracting the field value into a local variable would improve readability and resolve the line length issue.
const field_value = &@field(self.value, field.name);
nodes[changed_idx] = try ChildST.tree.fromValue(self.pool, field_value);
References
- Lines must not exceed 100 columns to ensure code is readable without horizontal scrolling. (link)
| // Phase 2: Splice nodes into the tree. | ||
| // setNodesAtDepth refs each node via rebind (refcount 0 → 1, tree-owned). | ||
| // On success, the tree owns the only ref — do NOT free the nodes. | ||
| const new_root = try self.root.setNodesAtDepth(self.pool, ST.chunk_depth, indices[0..changed_idx], nodes[0..changed_idx]); |
There was a problem hiding this comment.
This line exceeds the 100-column limit specified in the style guide (line 400). Please wrap the function call for better readability.
const new_root = try self.root.setNodesAtDepth(
self.pool,
ST.chunk_depth,
indices[0..changed_idx],
nodes[0..changed_idx],
);
References
- Lines must not exceed 100 columns to ensure code is readable without horizontal scrolling. (link)
| if (std.mem.eql(u8, std.mem.asBytes(&@field(self.value, field_name)), std.mem.asBytes(&value))) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This line exceeds the 100-column limit specified in the style guide (line 400). Extracting the byte slices into local variables would make the code more readable and fix the line length.
const current_bytes = std.mem.asBytes(&@field(self.value, field_name));
const new_bytes = std.mem.asBytes(&value);
if (std.mem.eql(u8, current_bytes, new_bytes)) {
return;
}
References
- Lines must not exceed 100 columns to ensure code is readable without horizontal scrolling. (link)
wemeetagain
force-pushed
the
cayman/struct-container-cleanup
branch
2 times, most recently
from
2a30ec4 to
33ed725
Compare
…Type Add StructContainerTreeView — a tree view for fixed-size containers that stores the value as a flat Zig struct. Fields are read and written in O(1) via direct struct access. Dirty fields are tracked with a StaticBitSet. On commit, only changed fields are spliced into the merkle tree via setNodesAtDepth. The pool has no special node types — this is purely a view-level optimization. - Add StructContainerTreeView with StaticBitSet dirty tracking - Parameterize FixedContainerType with TreeViewType enum - StructContainerType selects StructContainerTreeView via the enum - Model phase0 Validator as StructContainerType - set() skips write and dirty-marking when value is unchanged - serializeIntoBytes/toValue read from the struct directly (no commit needed) - commit has clear ownership phases with correct errdefer cleanup Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
wemeetagain
force-pushed
the
cayman/struct-container-cleanup
branch
from
33ed725 to
458802d
Compare
| changed_idx = 0; // disarm errdefer — nodes are now tree-owned | ||
|
|
||
| // Phase 3: Update root ownership. | ||
| errdefer self.pool.unref(new_root); |
There was a problem hiding this comment.
errdefer should be below the next try self.pool.ref(new_root); when we're sure it's ref successfully
but then there is no try after that
so seems like a redundant errdefer here
| pub fn fromValue(allocator: Allocator, pool: *Node.Pool, value: *const ST.Type) !*Self { | ||
| // tree.fromValue returns root at refcount 0. Use init which refs it. | ||
| const root = try ST.tree.fromValue(pool, value); | ||
| const ptr = try Self.init(allocator, pool, root); |
There was a problem hiding this comment.
note that this call will extract value from root again but not sure how to improve it
the TreeView.init() is a contract anyway
There was a problem hiding this comment.
maybe inline the whole logic of init here, similar to what we've done in clone() above?
There was a problem hiding this comment.
@wemeetagain the performance of before_process_epoch is almost the same to main
this is due to having to deserialize value from a node
lodestar-z/src/ssz/tree_view/chunks.zig
Line 335 in ce6370e
while with #232 you just get it from the pointer of node https://github.com/ChainSafe/lodestar-z/pull/232/changes#diff-2320e93e35c98f65b4cef2c729d8729fda9c280365baa569c2bf6c961230d745R839
- clone() respects transfer_cache option - fromValue() inlines init logic to skip redundant tree readback - Remove redundant errdefer in commit() after pool.ref Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
lodekeeper-z
left a comment
lodekeeper-z
left a comment
There was a problem hiding this comment.
Review: feat: add StructContainerTreeView, model Validator as StructContainerType
Clean design. The core insight — caching the full struct value alongside the merkle tree and using a StaticBitSet dirty tracker to splice only changed fields on commit — gives O(1) field access without touching the pool's node model. This is a strictly view-level optimization: no new node types, no pointer encoding in left/right slots, no BranchStructRef indirection. That's a meaningful simplification over the competing approach in #232.
What's good
-
Zero pool coupling. The pool doesn't know or care about
StructContainerTreeView. It sees normal leaf/branch nodes. This means every existing operation —setNodesAtDepth,getNodeAtDepth,clone, serialization — works unchanged. No new node lifecycle to reason about. No type-erased vtable stored in pointer-encoded left/right slots. -
StructContainerTypeviaTreeViewTypeenum. The parameterization ofFixedContainerTypeWithto select betweenContainerTreeViewandStructContainerTreeViewis minimal and clean. The SSZ type layer is unchanged —StructContainerTypedelegates everything toFixedContainerTypeexcept which tree view to use. -
Commit is well-structured. The three-phase approach (create leaf nodes → splice into tree → update root ownership) with
errdefercleanup and thechanged_idx = 0disarm trick is correct. The ownership chain is clear: nodes start at refcount 0,setNodesAtDepthtakes ownership via rebind, then we ref the new root and unref the old. -
set()skip-if-unchanged. The byte-level equality check inset()avoids dirtying unchanged fields. Smart forprocess_epochwhere many validators get "written" with the same value. -
Test suite. Covers the important cases — partial update correctness, clone isolation, pool node leak detection (tight pool test), double-commit idempotency, deinit with uncommitted changes, fromValue roundtrip.
Observations
1. toValue skips commit — is this intentional?
pub fn toValue(self: *Self, allocator: Allocator, out: *ST.Type) !void {
_ = allocator;
out.* = self.value;
}This returns the in-memory value directly, which includes uncommitted changes (since set() writes to self.value immediately). That's actually fine — the struct is the source of truth, and uncommitted just means the tree hasn't been updated yet. But serializeIntoBytes also reads from self.value without committing, so serialization of a view with uncommitted changes gives the dirty value, not the tree's value. This is consistent but worth a doc comment clarifying that toValue/serializeIntoBytes reflect uncommitted state while hashTreeRoot/getFieldRoot commit first.
2. Memory size per view: ~sizeof(T) + bitset overhead
For Validator (121 bytes as SSZ, struct is 129 bytes with Zig padding), each StructContainerTreeView carries the full struct. With ~1M validators in a ListCompositeTreeView, how does this interact? Each get() call on the list creates a new StructContainerTreeView (via init) which does toValue (reads 8 fields from the tree). If process_epoch accesses many validators, we're paying for the tree readback on every list access.
I suspect the real perf win comes from combining this with twoeths's observation: skipping the per-field deserialization by reading directly from a branch_struct node. Your approach is a stepping stone — the view API is right, but the tree-backed init path will need the pointer-in-node optimization from #232 for the hot loop. Is that the intended roadmap? #244 ("Cayman/232 review") suggests you're working on reconciling the two approaches.
3. fromValue double-builds the tree
The inlined fromValue builds the full merkle tree via ST.tree.fromValue(pool, value) (creating all 8 leaf nodes + branch nodes), then stores value.* in the struct. On the next commit(), if any field is dirty, it creates new leaf nodes and splices them in. The initial tree nodes for those fields are then unreachable and freed. Not a bug, but if fromValue is followed immediately by mutations (common pattern: create validator, set fields), the initial tree work for the mutated fields is wasted. An optimization for later: fromValue could skip tree building entirely and defer it to the first commit().
4. Devil's advocate: when does this approach lose to #232?
#232 stores the struct pointer directly in the node pool (via branch_struct node type), so any code that navigates the tree to the validator node gets O(1) access to the struct without creating a view. In your approach, every access requires instantiating a StructContainerTreeView, doing the tree readback, and heap-allocating the view. For one-off reads ("get validator N's effective balance"), #232 wins.
Your approach wins on simplicity, correctness, and non-invasiveness. The pool stays clean. But for process_epoch with ~1M validators, the per-access overhead matters. The two approaches may actually compose: use #232's branch_struct for storage + your StructContainerTreeView API for mutations. Worth discussing.
Nits
- Several lines exceed 100 columns (the
set()byte comparison,commit()field iteration). Gemini flagged these — worth wrapping for style guide compliance. toValueignoresallocatorparam. Consider documenting why (or removing it if the interface allows — thoughassertTreeViewTypeprobably requires it).
vs #232
For anyone following both PRs: #247 is the simpler, safer approach (view-level only, no pool changes). #232 is more invasive (new branch_struct node type, pointer encoding, type-erased vtable in the pool) but gives O(1) access without view instantiation. twoeths's benchmark comment on #247 suggests performance is "almost the same as main" for before_process_epoch because the init path still deserializes from the tree — this confirms the readback cost needs addressing regardless of which approach lands.
I'd suggest landing #247 first (it's simpler, correct, well-tested, and the view API is right), then layering #232's branch_struct storage optimization underneath if benchmarks justify it.
Overall: clean, correct, well-tested. 👍
3 participants
Summary
Replaces PR #232. Adds
StructContainerTreeView— a view-level optimization for fixed-size containers that stores the value as a flat Zig struct instead of decomposing it into the merkle tree.The optimization: fields are read and written in O(1) via direct struct field access. Dirty fields are tracked with a
StaticBitSet. On commit, only changed fields are spliced into the existing merkle tree viasetNodesAtDepth— changing 1 field out of 8 creates ~3 pool nodes instead of ~15.serializeIntoBytesandtoValueread directly from the struct without committing.The pool stays pure. No special node types, no type erasure, no pointer stuffing. This is entirely a view-layer optimization. The tree in the pool is a normal merkle tree built by
FixedContainerType.tree.fromValue.Design
StructContainerTreeViewstoresvalue: T(owned flat struct) +changed: StaticBitSet(N)(1 byte for Validator's 8 fields)getis a single@field(self.value, name)— no tree traversal, no optional unwrapsetwrites to the struct and sets a bit — skips if value unchangedcommititerates set bits, creates leaf nodes only for dirty fields, splices viasetNodesAtDepthStructContainerTypeisFixedContainerTypeWith(ST, .struct_container)— 3 lines, no delegation boilerplateWhat changed
src/ssz/type/container.zigFixedContainerTypewithTreeViewTypeenum; addStructContainerTypesrc/ssz/tree_view/container.zigStructContainerTreeViewwith testssrc/consensus_types/phase0.zigStructContainerTypesrc/state_transition/test_utils/generate_state.zigTest plan
zig build test:ssz— all tests pass including:zig build test:spec_tests -Dpreset=minimal🤖 Generated with Claude Code