feat: Parallel updates to Merkle trie (v2) #1258
Conversation
…s/firewood into bernard/insert-worker-pool-rayon
worker thread compiles (not tested).
a new branch node with an empty partial path is added to support parallel insertion. Appears to be working.
trie after the insert operations.
…-pool-rayon-part2-merged
…llel does not need a mutable reference.
…to accept a NibblesIterator instead of a Path. The key for an operation is passed to a worker as a Box<[u8]> instead of a Path.
…-pool-rayon-part2-merged
Signed-off-by: bernard-avalabs <[email protected]>
|
This needs the lockfile updated. Please see #1321 for some instructions as this changed fairly recently. |
| // Check if the root has a value or if there is more than one child. If yes, then | ||
| // just return the root unmodified | ||
| if branch.value.is_some() || children_iter.next().is_some() { | ||
| return Ok(Some((*branch).into())); |
There was a problem hiding this comment.
This unboxes and boxes the branch, which means allocate, copy, free. Use a move instead, which might mean you need a new method on Node (maybe impl From<Box<BranchNode>>>) or just Node::Branch(branch) might work.
| let mut merkle = Merkle::from(worker_nodestore); | ||
|
|
||
| // Wait for a message on the receiver child channel. Break out of loop if there is an error. | ||
| while let Ok(request) = child_receiver.recv() { |
There was a problem hiding this comment.
Closing the sender without sending Done should be caught here, at least a log entry.
|
|
||
| /// Get a worker from the worker pool based on the `first_nibble` value. Create a worker if | ||
| /// it doesn't exist already. | ||
| fn get_worker( |
There was a problem hiding this comment.
nit: rust avoids naming things "get". Just worker would be fine here.
Why this isn't caught in clippy is a good question.
| .send(Request::DeleteRange { | ||
| prefix: Box::default(), // Empty prefix | ||
| }) | ||
| .expect("TODO: handle error"); |
There was a problem hiding this comment.
Let's handle this error better than panic.
| .build() | ||
| .expect("Error in creating threadpool") |
There was a problem hiding this comment.
Error handling can be better here, maybe:
| .build() | |
| .expect("Error in creating threadpool") | |
| .build()? |
| .expect("Should have a root node after prepare step") | ||
| .into_branch() | ||
| .expect("Root should be a branch after prepare step"); |
There was a problem hiding this comment.
This raises some concerns about the type system but I don't have a good easy solution.
| key: op.key().as_ref().into(), | ||
| value: value.as_ref().into(), | ||
| }) | ||
| .expect("send to worker error"); |
There was a problem hiding this comment.
These expects should also get cleaned up. We want to avoid panics and instead return errors back. Otherwise, the root cause of the problem will be hard to identify.
Same for the other cases below.
| self.workers = [(); BranchNode::MAX_CHILDREN].map(|()| None); | ||
|
|
||
| let immutable: Arc<NodeStore<Arc<ImmutableProposal>, FileBacked>> = | ||
| Arc::new(proposal.try_into().expect("error creating immutable")); |
There was a problem hiding this comment.
This can fail, and we should return that error. An example would be a giant node due to too large of a value.
| pub fn take_child(&mut self, child_index: u8) -> Option<Child> { | ||
| self.children | ||
| .get_mut(child_index as usize) | ||
| .expect("index error") | ||
| .take() | ||
| } |
There was a problem hiding this comment.
Ideally, we don't call expect here. Brandon recently made some changes here that might make it easier.
2 participants
Second implementation of adding parallel updates to a Merkle trie. In this approach, each child of the root node is a separate trie that can be modified independently by a separate worker thread. Four steps are needed to create a parallel proposal:
The main changes are in parallel.rs. A test case (test_propose_parallel) has been added to test parallel insertion/deletion.