Skip to content
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.

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

Trie hash optimizations #5827

Merged
merged 3 commits into from
Dec 26, 2023
Merged

Trie hash optimizations #5827

merged 3 commits into from
Dec 26, 2023

Conversation

dvush
Copy link
Contributor

@dvush dvush commented Dec 19, 2023

This PR improves a performance of the trie when calculating hash.

In my benchmark it reduces time from 100ms to 90ms and this 10ms are more noticeable in implementation that caches db reads for the trie.

The most influential commits are:

  • cursor subnode full_key cache
    This key clone alone is mostly responsible for 10ms drop
  • AHash in trie
    Recently hashbrown was removed from the repo and it made all hash maps / sets much slower. Its mostly due to hashbrown using ahash for hashing while std is using siphash. This only changes hashmaps in trie related code but other parts of reth can benefit from it too.
  • prealloc children is not that noticeable but improves push_branch_node a bit. (without it about 20% of the method is spent doing this collect) (moved to prealloc children alloy-rs/trie#1)

@dvush
Copy link
Contributor Author

dvush commented Dec 19, 2023

Depends on alloy-rs/nybbles#1

@onbjerg
Copy link
Member

onbjerg commented Dec 19, 2023

cc @DaniPopes this should be in alloy-trie, correct?

@DaniPopes
Copy link
Member

DaniPopes commented Dec 20, 2023

cc @DaniPopes this should be in alloy-trie, correct?

Only the primitives change. If you don't mind you can open a PR to https://github.com/alloy-rs/trie with the prealloc commit @dvush

crates/trie/src/walker.rs Outdated Show resolved Hide resolved
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

LGTM after comments are resolved. cc @rkrasiuk

@rkrasiuk rkrasiuk added C-perf A change motivated by improving speed, memory usage or disk footprint A-trie Related to Merkle Patricia Trie implementation labels Dec 26, 2023
@rkrasiuk
Copy link
Member

@dvush in any follow-up perf PRs it would be great to see side by side comparison of flamegraphs / benches before and after the change. otherwise, it's tough to verify the performance boost from the changes. even some obvious "improvements" might degrade the performance as we've experienced in the past

Copy link
Member

@rkrasiuk rkrasiuk left a comment

Choose a reason for hiding this comment

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

overall lgtm modulo my comment re perf PRs above. tested at mainnet tip and ran execution debug script on holesky

@rkrasiuk rkrasiuk added this pull request to the merge queue Dec 26, 2023
Merged via the queue into paradigmxyz:main with commit abc168e Dec 26, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trie Related to Merkle Patricia Trie implementation C-perf A change motivated by improving speed, memory usage or disk footprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants