feat(delayed-persist): Part 1: Roots may be in mem #1041

rkuris · 2025-07-07T23:14:40Z

Introduces a new type, MaybePersistedNode, which is a node which is either a SharedNode or a LinearAddress. This new type will allow writes to occur while readers are still reading this revision using an ArcSwap.

Note that this does not actually put any additional nodes in memory, it just sets things up so they could be in memory.

Additional changes needed for this feature to be complete are:

BranchNode children can now be Node, LinearAddress or a MaybePersistedNode
When converting a MutableProposal to an ImmutableProposal, don't actually allocate space, just create SharedNodes from the Nodes.
Remove NodeStoreHeader from NodeStore. The header should move into the RevisionManager.
Allocate new nodes from the recently-freed nodes from an expired revision, then from the freelist. This can be done by maintaining a local freelist from the nodes deleted in the expiring revision and falling back to the actual freelist. Deleted nodes that are not reused must still be pushed onto the freelist.

Introduces a new type, `MemDiskNode`, which is a node which is either a SharedNode or a LinearAddress. This new type will allow writes to occur while readers are still reading this revision using an `ArcSwap`. Note that this does not actually put any additional nodes in memory, it just sets things up so they could be in memory. Additional changes needed for this feature to be complete are: - [ ] BranchNode children can now be Node, LinearAddress or MemDiskNode - [ ] When converting a `MutableProposal` to an `ImmutableProposal`, don't actually allocate space, just create `SharedNode`s from the `Node`s. - [ ] Remove `NodeStoreHeader` from `NodeStore`. The header should move into the `RevisionManager`. - [ ] Allocate new nodes from the recently-freed nodes from an expired revision, then from the freelist. This can be done by maintaining a local freelist from the nodes deleted in the expiring revision and falling back to the actual freelist. Deleted nodes that are not reused must still be pushed onto the freelist.

storage/src/node/memdisk.rs

rkuris · 2025-07-08T06:05:05Z

The goal of this set of diffs is to separate persistence from both proposals and committed revisions. When proposing, we currently allocate space and modify the freelist. When committing, we currently flush all the modified nodes to disk.

This diff is step 1. The only thing this diff does is allow for the root node (and only the root node) of proposals and revisions to be kept in memory and, using a lockless type, allow them to be persisted even while being read by other threads. It doesn't do that, since they are currently always created as an allocated node, but that will change on future diffs.

In other words, it still always marks the node as persisted by assigning the address at propose time and using the persisted variant of the new MemDiskNode type when created, doesn't allow for branches to have their changes in memory, and doesn't defer the allocation to commit time. So this means this diff doesn't actually make any improvements. It might have a performance impact and it might be good to measure it at this point though.

This is a better name.

storage/src/nodestore.rs

storage/src/node/memdisk.rs

qusuyan · 2025-07-08T15:37:37Z

storage/src/node/memdisk.rs

+    /// # Arguments
+    ///
+    /// * `addr` - The `LinearAddress` where the node has been persisted on disk
+    pub fn persist_at(&self, addr: LinearAddress) {


Sounds like this function is really called when the node is evicted from cache instead of when it gets persisted. Maybe rename?

No, it's called after the. node is written to disk.

Oh because after you persist it you can read it from the cache of linear storage using its address right? Then I think MaybeAllocated may be a better name than MemDiskNode.

It's not just allocated, it's persisted. The states here are:

Node in memory (SharedNode)

Node on disk (LinearAddress)

Allocation is the verb that describes part of persisting a node. First you allocate space, then you write (persist) it. If it has a LinearAddress, and I'm done with all my changes, the only thing that will have LinearAddresses are persisted. Right now we get a LinearAddress at allocation time, which is at propose time, but that's all moving.

Upcoming changes make supporting this harder, and it's not necessary, as the size of a LinearAddress and the size of a pointer are the same, so it's more efficient to return the actual value rather than a pointer to the value.

Members renamed from Mem to Unpersisted and Disk to Persisted, just to keep the naming a bit more consistent.

storage/src/node/persist.rs

+/// - `Unpersisted(SharedNode)`: The node is currently in memory
+/// - `Persisted(LinearAddress)`: The node is currently on disk at the specified address
+#[derive(Debug)]
+pub enum MaybePersisted {


Most important test is that the node is no longer referenced after it becomes persisted, and it's shared across clones.

Create a table explaining the type more clearly. Lower visibility of MaybePersisted.

Branch children might also not be persisted. This also splits up hash_helper which got too long for clippy. - [x] Roots may not be persisted (Part 1: #1041) - [x] BranchNode children can now be Node, LinearAddress or a MaybePersistedNode (Part 2: #1045 and this PR #1047) - [ ] When converting a `MutableProposal` to an `ImmutableProposal`, don't actually allocate space, just create `SharedNode`s from the `Node`s. - [ ] Remove `NodeStoreHeader` from `NodeStore`. The header should move into the `RevisionManager`. - [ ] Allocate new nodes from the recently-freed nodes from an expired revision, then from the freelist. This can be done by maintaining a local freelist from the nodes deleted in the expiring revision and falling back to the actual freelist. Deleted nodes that are not reused must still be pushed onto the freelist.

This implements deferred allocaion that delays allocating space for nodes until commit time. The nodestore refactor consisted of removing the 'new' hashmap, which mapped newly allocated nodes to their contents in a proposal. Instead, now we store those newly allocated nodes directly from the `Child` struct. The commit logic is also different, with far fewer steps. See the comments in `commit` in manager.rs. Task completion status: [x] Roots may not be persisted (Part 1: #1041) [x] BranchNode children can now be Node, LinearAddress or a MaybePersistedNode (Part 2: #1045 and #1047) [x] When converting a `MutableProposal` to an `ImmutableProposal`, don't actually allocate space, just create `SharedNode`s from the `Node`s. (Part 3: #1055 and this PR) [ ] Remove `NodeStoreHeader` from `NodeStore`. The header should move into the `RevisionManager`. [ ] Allocate new nodes from the recently-freed nodes from an expired revision, then from the freelist. This can be done by maintaining a local freelist from the nodes deleted in the expiring revision and falling back to the actual freelist. Deleted nodes that are not reused must still be pushed onto the freelist.

Branch children might also not be persisted. This also splits up hash_helper which got too long for clippy. - [x] Roots may not be persisted (Part 1: ava-labs#1041) - [x] BranchNode children can now be Node, LinearAddress or a MaybePersistedNode (Part 2: ava-labs#1045 and this PR ava-labs#1047) - [ ] When converting a `MutableProposal` to an `ImmutableProposal`, don't actually allocate space, just create `SharedNode`s from the `Node`s. - [ ] Remove `NodeStoreHeader` from `NodeStore`. The header should move into the `RevisionManager`. - [ ] Allocate new nodes from the recently-freed nodes from an expired revision, then from the freelist. This can be done by maintaining a local freelist from the nodes deleted in the expiring revision and falling back to the actual freelist. Deleted nodes that are not reused must still be pushed onto the freelist.

This implements deferred allocaion that delays allocating space for nodes until commit time. The nodestore refactor consisted of removing the 'new' hashmap, which mapped newly allocated nodes to their contents in a proposal. Instead, now we store those newly allocated nodes directly from the `Child` struct. The commit logic is also different, with far fewer steps. See the comments in `commit` in manager.rs. Task completion status: [x] Roots may not be persisted (Part 1: #1041) [x] BranchNode children can now be Node, LinearAddress or a MaybePersistedNode (Part 2: #1045 and #1047) [x] When converting a `MutableProposal` to an `ImmutableProposal`, don't actually allocate space, just create `SharedNode`s from the `Node`s. (Part 3: #1055 and this PR) [ ] Remove `NodeStoreHeader` from `NodeStore`. The header should move into the `RevisionManager`. [ ] Allocate new nodes from the recently-freed nodes from an expired revision, then from the freelist. This can be done by maintaining a local freelist from the nodes deleted in the expiring revision and falling back to the actual freelist. Deleted nodes that are not reused must still be pushed onto the freelist.

Create a new type, NodeAllocator, to handle allocations. Currently only uses references to the original NodeStore, but this will change in an upcoming diff. Also: - Branch children iterators will soon no longer be able to return references to hashes, since they are behind an ArcSwap, so they are now cloned. - Added logger feature to benchmark (helped with debugging) - Remove use of stored_len in tests feat(deferred-allocate): Part 3.2: Implementation This implements deferred allocaion that delays allocating space for nodes until commit time. The nodestore refactor consisted of removing the 'new' hashmap, which mapped newly allocated nodes to their contents in a proposal. Instead, now we store those newly allocated nodes directly from the `Child` struct. The commit logic is also different, with far fewer steps. See the comments in `commit` in manager.rs. Task completion status: [x] Roots may not be persisted (Part 1: #1041) [x] BranchNode children can now be Node, LinearAddress or a MaybePersistedNode (Part 2: #1045 and #1047) [x] When converting a `MutableProposal` to an `ImmutableProposal`, don't actually allocate space, just create `SharedNode`s from the `Node`s. (Part 3: #1055 and this PR) [ ] Remove `NodeStoreHeader` from `NodeStore`. The header should move into the `RevisionManager`. [ ] Allocate new nodes from the recently-freed nodes from an expired revision, then from the freelist. This can be done by maintaining a local freelist from the nodes deleted in the expiring revision and falling back to the actual freelist. Deleted nodes that are not reused must still be pushed onto the freelist.

This implements deferred allocaion that delays allocating space for nodes until commit time. The nodestore refactor consisted of removing the 'new' hashmap, which mapped newly allocated nodes to their contents in a proposal. Instead, now we store those newly allocated nodes directly from the `Child` struct. The implementation constisted of: - A new type, NodeAllocator, to consolidate all the allocation code - Removing the 'new' hashmap, as freshly allocatred nodes are found by traversing the trie and inspecting their state - Rewrite flush_nodes to serialize then allocate. Previously, the node was serialized twice, once to determine the size during proposal creation and once to store it - Branch children iterators can no longer return reference to hashes, since they are sometimes behind an ArcSwap - Changes to the commit flow by separating the node persistence logic Some miscellaneous changes included in this diff (could be split out): - Propogate logger feature in fwdctl (helped with debugging) - Remove use of stored_len in tests - size_from_area_index was a method on nodestore, but didn't use nodestore - uncached_node_and_size was dead code Task completion status: [x] Roots may not be persisted (Part 1: #1041) [x] BranchNode children can now be Node, LinearAddress or a MaybePersistedNode (Part 2: #1045 and #1047) [x] When converting a `MutableProposal` to an `ImmutableProposal`, don't actually allocate space, just create `SharedNode`s from the `Node`s. (Part 3: #1055 and this PR) [ ] Remove `NodeStoreHeader` from `NodeStore`. The header should move into the `RevisionManager`. [ ] Allocate new nodes from the recently-freed nodes from an expired revision, then from the freelist. This can be done by maintaining a local freelist from the nodes deleted in the expiring revision and falling back to the actual freelist. Deleted nodes that are not reused must still be pushed onto the freelist.

This implements deferred allocaion that delays allocating space for nodes until commit time. The nodestore refactor consisted of removing the 'new' hashmap, which mapped newly allocated nodes to their contents in a proposal. Instead, now we store those newly allocated nodes directly from the `Child` struct. The implementation constisted of: - A new type, NodeAllocator, to consolidate all the allocation code - Removing the 'new' hashmap, as freshly allocatred nodes are found by traversing the trie and inspecting their state - Use the most recently committed revision's freelist to allocate new nodes. This will change in a future revision when the freelist is tied to storage rather than the revision. - Rewrite flush_nodes to serialize then allocate. Previously, the node was serialized twice, once to determine the size during proposal creation (allocation time) and once to store it - Changes to the commit flow by separating the node persistence logic (see comments in commit.rs) Some miscellaneous changes included in this diff (could be split out): - Branch children iterators can no longer return reference to hashes, since they are sometimes behind an ArcSwap - Propogate logger feature in fwdctl (helped with debugging) - Remove use of stored_len in tests - size_from_area_index was a method on nodestore, but didn't use nodestore - uncached_node_and_size was dead code Task completion status: [x] Roots may not be persisted (Part 1: #1041) [x] BranchNode children can now be Node, LinearAddress or a MaybePersistedNode (Part 2: #1045 and #1047) [x] When converting a `MutableProposal` to an `ImmutableProposal`, don't actually allocate space, just create `SharedNode`s from the `Node`s. (Part 3: #1055 and this PR) [ ] Remove `NodeStoreHeader` from `NodeStore`. The header should move into the `RevisionManager`. [ ] Allocate new nodes from the recently-freed nodes from an expired revision, then from the freelist. This can be done by maintaining a local freelist from the nodes deleted in the expiring revision and falling back to the actual freelist. Deleted nodes that are not reused must still be pushed onto the freelist.

This implements deferred allocaion that delays allocating space for nodes until commit time. The nodestore refactor consisted of removing the 'new' hashmap, which mapped newly allocated nodes to their contents in a proposal. Instead, now we store those newly allocated nodes directly from the `Child` struct. The implementation constisted of: - A new type, NodeAllocator, to consolidate all the allocation code - Removing the 'new' hashmap, as freshly allocatred nodes are found by traversing the trie and inspecting their state - Use the most recently committed revision's freelist to allocate new nodes. This will change in a future revision when the freelist is tied to storage rather than the revision. - Rewrite flush_nodes to serialize then allocate. Previously, the node was serialized twice, once to determine the size during proposal creation (allocation time) and once to store it - Changes to the commit flow by separating the node persistence logic (see comments in commit.rs) Some miscellaneous changes included in this diff (could be split out): - Branch children iterators can no longer return reference to hashes, since they are sometimes behind an ArcSwap - Propogate logger feature in fwdctl (helped with debugging) - Remove use of stored_len in tests - size_from_area_index was a method on nodestore, but didn't use nodestore - uncached_node_and_size was dead code - New clippy lint for hash_helper not needing &self in non-ethhash fixed Task completion status: [x] Roots may not be persisted (Part 1: #1041) [x] BranchNode children can now be Node, LinearAddress or a MaybePersistedNode (Part 2: #1045 and #1047) [x] When converting a `MutableProposal` to an `ImmutableProposal`, don't actually allocate space, just create `SharedNode`s from the `Node`s. (Part 3: #1055 and this PR) [ ] Remove `NodeStoreHeader` from `NodeStore`. The header should move into the `RevisionManager`. [ ] Allocate new nodes from the recently-freed nodes from an expired revision, then from the freelist. This can be done by maintaining a local freelist from the nodes deleted in the expiring revision and falling back to the actual freelist. Deleted nodes that are not reused must still be pushed onto the freelist.

feat(deferred-allocate): Part 3: Defer allocate This implements deferred allocaion that delays allocating space for nodes until commit time. The nodestore refactor consisted of removing the 'new' hashmap, which mapped newly allocated nodes to their contents in a proposal. Instead, now we store those newly allocated nodes directly from the `Child` struct. The implementation constisted of: - A new type, NodeAllocator, to consolidate all the allocation code - Removing the 'new' hashmap, as freshly allocatred nodes are found by traversing the trie and inspecting their state - Use the most recently committed revision's freelist to allocate new nodes. This will change in a future revision when the freelist is tied to storage rather than the revision. - Rewrite flush_nodes to serialize then allocate. Previously, the node was serialized twice, once to determine the size during proposal creation (allocation time) and once to store it - Changes to the commit flow by separating the node persistence logic (see comments in commit.rs) Some miscellaneous changes included in this diff (could be split out): - Branch children iterators can no longer return reference to hashes, since they are sometimes behind an ArcSwap - Propogate logger feature in fwdctl (helped with debugging) - Remove use of stored_len in tests - size_from_area_index was a method on nodestore, but didn't use nodestore - uncached_node_and_size was dead code - New clippy lint for hash_helper not needing &self in non-ethhash fixed Task completion status: [x] Roots may not be persisted (Part 1: #1041) [x] BranchNode children can now be Node, LinearAddress or a MaybePersistedNode (Part 2: #1045 and #1047) [x] When converting a `MutableProposal` to an `ImmutableProposal`, don't actually allocate space, just create `SharedNode`s from the `Node`s. (Part 3: #1055 and this PR) [ ] Remove `NodeStoreHeader` from `NodeStore`. The header should move into the `RevisionManager`. [ ] Allocate new nodes from the recently-freed nodes from an expired revision, then from the freelist. This can be done by maintaining a local freelist from the nodes deleted in the expiring revision and falling back to the actual freelist. Deleted nodes that are not reused must still be pushed onto the freelist.

rkuris requested review from qusuyan and alarso16 July 7, 2025 23:14

rkuris requested review from aaronbuchwald and demosdemon as code owners July 7, 2025 23:14

rkuris removed the request for review from aaronbuchwald July 7, 2025 23:17

rkuris commented Jul 8, 2025

View reviewed changes

storage/src/node/memdisk.rs Outdated Show resolved Hide resolved

Rename allocate to persist_at

65b9200

This is a better name.

qusuyan requested changes Jul 8, 2025

View reviewed changes

Rename MemDiskNode to MaybePersistedNode

deeb99b

qusuyan approved these changes Jul 8, 2025

View reviewed changes

rkuris requested a review from qusuyan July 8, 2025 16:24

rkuris force-pushed the rkuris/memory-root branch from c51a807 to c31a9ce Compare July 8, 2025 16:32

rkuris added 3 commits July 8, 2025 09:43

fix: Avoid reference to LinearAddress (#1042)

f51cf74

Upcoming changes make supporting this harder, and it's not necessary, as the size of a LinearAddress and the size of a pointer are the same, so it's more efficient to return the actual value rather than a pointer to the value.

Rename memdisk.rs to persist.rs

1fcbdb0

Rename MaybePersistedEnum to MaybePersisted

7575745

Members renamed from Mem to Unpersisted and Disk to Persisted, just to keep the naming a bit more consistent.

rkuris force-pushed the rkuris/memory-root branch from c31a9ce to 7575745 Compare July 8, 2025 16:43

Merge branch 'main' into rkuris/memory-root

3dfd7ee

demosdemon approved these changes Jul 8, 2025

View reviewed changes

storage/src/node/persist.rs Outdated Show resolved Hide resolved

Merge branch 'main' into rkuris/memory-root

c6b3439

demosdemon reviewed Jul 8, 2025

View reviewed changes

storage/src/node/persist.rs Outdated

/// - `Unpersisted(SharedNode)`: The node is currently in memory

/// - `Persisted(LinearAddress)`: The node is currently on disk at the specified address

#[derive(Debug)]

pub enum MaybePersisted {

This comment was marked as resolved.

Sign in to view

rkuris added 2 commits July 8, 2025 11:28

Add testing for MaybePersistNode

0e91769

Most important test is that the node is no longer referenced after it becomes persisted, and it's shared across clones.

Reviewer comments

bf91808

Create a table explaining the type more clearly. Lower visibility of MaybePersisted.

rkuris enabled auto-merge (squash) July 8, 2025 19:26

persist_at must be exported

2a6be0e

rkuris merged commit d9e93c9 into main Jul 8, 2025
34 checks passed

rkuris deleted the rkuris/memory-root branch July 8, 2025 19:40

rkuris mentioned this pull request Jul 9, 2025

feat(delayed-persist): Part 2.2: Branch Children #1047

Merged

5 tasks

rkuris mentioned this pull request Jul 15, 2025

feat(deferred-allocate): Part 3: Defer allocate #1061

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(delayed-persist): Part 1: Roots may be in mem #1041

feat(delayed-persist): Part 1: Roots may be in mem #1041

Uh oh!

rkuris commented Jul 7, 2025 •

edited

Loading

Uh oh!

Uh oh!

rkuris commented Jul 8, 2025

Uh oh!

Uh oh!

Uh oh!

qusuyan Jul 8, 2025

Uh oh!

rkuris Jul 8, 2025

Uh oh!

qusuyan Jul 8, 2025

Uh oh!

rkuris Jul 8, 2025

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

feat(delayed-persist): Part 1: Roots may be in mem #1041

feat(delayed-persist): Part 1: Roots may be in mem #1041

Uh oh!

Conversation

rkuris commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rkuris commented Jul 8, 2025

Uh oh!

Uh oh!

Uh oh!

qusuyan Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

rkuris Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

qusuyan Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

rkuris Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

rkuris commented Jul 7, 2025 •

edited

Loading