Cayman/232 review - #244
Conversation
- Remove branch_struct_lazy/computed node types from Node.Pool - Restore 2-bit type field / 29-bit ref count - Remove BranchStructRef, createBranchStruct, getStructPtr - StructContainerType.tree delegates to FixedContainerType.tree - StructContainerTreeView stores owned T value instead of borrowing from pool; init reads from merkle tree, commit writes back Pool stays pure: only zero/leaf/branch nodes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add TreeViewState struct with concrete typed methods for the common state (allocator, pool, root, children_nodes, changed) shared by all chunk-based tree views. Replaces duck-typed ChildNodes anytype dispatch. - BasicPackedChunks, CompositeChunks, BitArray embed TreeViewState - Update all callers (list/array/bit views) to access via .state.* - Delete child_nodes.zig utility Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 significantly refactors the internal architecture of tree views and the node pool. The primary motivation is to enforce a clear separation of concerns: the node pool is now strictly an arena for generic tree nodes, while view-specific optimizations and data management are handled at the view layer. This change enhances the purity and predictability of the node pool, improves type safety and code reuse across various tree view implementations through a new centralized state struct, and optimizes change tracking for container views. 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 is a significant and well-executed refactoring. It simplifies the Node.Pool by removing the branch_struct type, which is a great improvement for design purity and maintainability. The introduction of TreeViewState to consolidate common state and logic from various tree views is another excellent change that enhances code structure and type safety. My review includes a couple of suggestions to improve code clarity and further align with the repository's style guide, particularly regarding safety assertions.
| const ptr = try allocator.create(Self); | ||
| ptr.* = .{ | ||
| .allocator = allocator, | ||
| .pool = pool, | ||
| .root = root, | ||
| .original_value = original_value, | ||
| .changed = null, | ||
| }; | ||
| errdefer allocator.destroy(ptr); | ||
|
|
||
| try ST.tree.toValue(root, pool, &ptr.original_value); | ||
|
|
||
| ptr.allocator = allocator; | ||
| ptr.pool = pool; | ||
| ptr.root = root; | ||
| ptr.changed = null; | ||
| return ptr; |
There was a problem hiding this comment.
This init function can be made more readable by separating the value deserialization from the struct initialization. This allows using a single struct literal for initialization, which is generally clearer than field-by-field assignment.
var original_value: T = undefined;
try ST.tree.toValue(root, pool, &original_value);
const ptr = try allocator.create(Self);
ptr.* = .{
.allocator = allocator,
.pool = pool,
.root = root,
.original_value = original_value,
.changed = null,
};
return ptr;
| pub const TreeViewState = struct { | ||
| allocator: Allocator, | ||
| pool: *Node.Pool, | ||
| root: Node.Id, | ||
|
|
||
| /// cached nodes for faster access of already-visited children | ||
| children_nodes: std.AutoHashMapUnmanaged(Gindex, Node.Id), | ||
|
|
||
| /// whether the corresponding child node/data has changed since the last update of the root | ||
| changed: std.AutoArrayHashMapUnmanaged(Gindex, void), |
There was a problem hiding this comment.
This new TreeViewState struct is a great abstraction. To further align with the repository's style guide on safety, consider adding assertions to validate function arguments, pre-conditions, and post-conditions. For example, deinit could assert the root is valid, setChildNode could assert the node ID is valid, and commitNodes could assert that the changed map is empty upon completion.
References
- The style guide mandates asserting all function arguments, return values, pre/postconditions, and invariants to improve safety. It suggests an average of at least two assertions per function. (link)
e9c9929 to
6f0c12b
Compare
|
2 participants
Opening this PR for discussion and visibility into the whole review. Will PR into #139 first with the relevant pieces.
std.StaticBitSetfor container tree view changed tracking.Below is a more verbose and prescriptive explanation of above:
StructContainerTreeViewwas leaking a view-level optimization into the node pool. The pool gained a 5th node type (branch_struct), type-erased vtables, and pointer-stuffing in left/right fields — all so one tree view could store flat structs. Thiscut the ref count budget in half and broke the invariant that left/right are always pool indices.
This branch pulls the struct data back up to the view layer where it belongs.
StructContainerTreeViewnow owns its value directly — reads from the merkle tree atinit, writes back at commit. The pool stays pure: zero/leaf/branch.The pool is an arena of value-typed nodes addressed by 32-bit indices. Every node is the same size, the same shape — a hash, a left index, a right index, a state word. No pointers, no indirection, no type erasure. You can walk, hash, diff,
serialize, and ref-count the entire tree with just integer arithmetic on a flat array. Cache-friendly, trivially serializable, no lifetime tracking beyond ref counts.
branch_structbroke that. A "node" could now secretly be a pointer to a heap-allocated, type-erased object with its own vtable. The pool had to ask "is this a real node or a pointer?" before every operation —unref,getRoot,noChild. The flatarray wasn't flat anymore; some slots were trampolines to somewhere else on the heap.
Keeping the pool pure means every node is interchangeable. No special cases in traversal, no hidden allocations, no pointer-chasing. The complexity of "some containers are stored as flat structs" lives entirely in the view layer, which is where
type-aware logic belongs. The pool just does trees.
Along the way, the scattered fields duplicated across every chunk-based view (
allocator,pool,root,children_nodes,changed) are consolidated into a concreteTreeViewStatestruct, replacing duck-typedanytypedispatch. AndContainerTreeView.changedbecomes aStaticBitSet— zero allocation for a comptime-known field count.The old
BaseTreeViewwas the right idea — share the common state — but it was a heap-allocated indirection (*TreeViewData). The refactor that removed it (PR refactor(ssz): drop BaseTreeView, use tuple for ContainerTreeView #139) was correct to kill that pointer, but it overcorrected: it scattered the five fieldsinto every view type and recovered sharing through
ChildNodes— a set of free functions that takeanytypeand duck-type their way toself.allocator,self.pool,self.root,self.children_nodes,self.changed.Duck typing works, but it's invisible. Nothing in the type system says "this struct is a valid
ChildNodestarget." A typo in a field name compiles fine until someone calls the function. The compiler error points at theChildNodesutility, not atthe struct that's missing a field. And you can't look at
BasicPackedChunksand see at a glance what interface it satisfies — you have to read every method body and mentally verify the field names match.TreeViewStateis an embedded value type — not a pointer, not a trait, just a struct stored inline in each view. When you seestate: TreeViewStateinBasicPackedChunks, you know exactly what state it carries and what operations are available. Themethods are on a concrete type, so the compiler gives you real errors at the call site. And there's one implementation to read, not a set of generic functions that reconstruct the interface from field names.
It's also the right boundary.
ContainerTreeViewdoesn't embedTreeViewStatebecause it genuinely has different storage — comptime tuples and fixed arrays instead of hashmaps. The struct makes the split explicit: chunk-based views shareTreeViewState, container views don't. That distinction was invisible when it was just "some types happen to have the right field names forChildNodesto work."