Skip to content

perf: skip Rayon on small Merkle tree levels #550

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nicole-graus wants to merge 1 commit into from
Closed

perf: skip Rayon on small Merkle tree levels #550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions crypto/crypto/src/merkle_tree/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,18 @@ pub trait IsMerkleTreeBackend {
/// tree will be built from and converts it to a list of leaf nodes.
fn hash_leaves(unhashed_leaves: &[Self::Data]) -> Vec<Self::Node> {
#[cfg(feature = "parallel")]
let iter = unhashed_leaves.par_iter();
#[cfg(not(feature = "parallel"))]
let iter = unhashed_leaves.iter();

iter.map(|leaf| Self::hash_data(leaf)).collect()
{
if unhashed_leaves.len() >= 1024 {
Copy link
Copy Markdown

@claude claude Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — Magic constant duplicated across files

1024 is also hardcoded in utils.rs:86. If the threshold is ever tuned, it must be changed in both places. Define a shared constant (e.g. in utils.rs or a new consts.rs) and reference it here:

Suggested change
if unhashed_leaves.len() >= 1024 {
if unhashed_leaves.len() >= PARALLEL_THRESHOLD {

return unhashed_leaves
.par_iter()
.map(|leaf| Self::hash_data(leaf))
.collect();
}
}
unhashed_leaves
.iter()
.map(|leaf| Self::hash_data(leaf))
.collect()
}

/// This function takes to children nodes and builds a new parent node.
Expand Down
37 changes: 28 additions & 9 deletions crypto/crypto/src/merkle_tree/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,36 @@ where
let (new_level_iter, children_iter) =
nodes[new_level_begin_index..level_end_index + 1].split_at_mut(new_level_length);

// Skip Rayon for small levels: the scheduling overhead exceeds
// computation for levels with fewer than 1024 nodes. This avoids
// hundreds of unnecessary task spawns from small FRI layer trees.
#[cfg(feature = "parallel")]
let parent_and_children_zipped_iter = new_level_iter
.into_par_iter()
.zip(children_iter.par_chunks_exact(2));
{
if new_level_length >= 1024 {
Copy link
Copy Markdown

@claude claude Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — Same magic constant as in traits.rs

Same threshold value should come from one named constant so the two sites stay in sync:

Suggested change
if new_level_length >= 1024 {
if new_level_length >= PARALLEL_THRESHOLD {

new_level_iter
.into_par_iter()
.zip(children_iter.par_chunks_exact(2))
.for_each(|(new_parent, children)| {
*new_parent = B::hash_new_parent(&children[0], &children[1]);
});
} else {
new_level_iter
.iter_mut()
.zip(children_iter.chunks_exact(2))
.for_each(|(new_parent, children)| {
*new_parent = B::hash_new_parent(&children[0], &children[1]);
});
}
}
#[cfg(not(feature = "parallel"))]
let parent_and_children_zipped_iter =
new_level_iter.iter_mut().zip(children_iter.chunks_exact(2));

parent_and_children_zipped_iter.for_each(|(new_parent, children)| {
*new_parent = B::hash_new_parent(&children[0], &children[1]);
});
{
new_level_iter
.iter_mut()
.zip(children_iter.chunks_exact(2))
.for_each(|(new_parent, children)| {
*new_parent = B::hash_new_parent(&children[0], &children[1]);
});
}
Comment on lines +93 to +110
Copy link
Copy Markdown

@claude claude Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — Sequential logic duplicated

The else branch (lines 93–100) and the #[cfg(not(feature = "parallel"))] block (lines 102–110) are byte-for-byte identical. If the sequential logic ever changes it must be updated twice. Extract into a small inline helper or macro, e.g.:

fn build_level_sequential<B: IsMerkleTreeBackend>(
    new_level_iter: &mut [B::Node],
    children_iter: &[B::Node],
) where B::Node: Clone {
    new_level_iter
        .iter_mut()
        .zip(children_iter.chunks_exact(2))
        .for_each(|(p, c)| *p = B::hash_new_parent(&c[0], &c[1]));
}

Then both sites call build_level_sequential::<B>(new_level_iter, children_iter);.


level_end_index = level_begin_index - 1;
level_begin_index = new_level_begin_index;
Expand Down
Loading