-
Notifications
You must be signed in to change notification settings - Fork 5
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
Added tooling for trie stats #8
Conversation
- Also did some refactoring to reduce code duplication that cut across modules
- Previously had a lot of redundant info that was also present in `TrieStats`. Now compares the depth values from both tries directly.
efece0e
to
2b72c7f
Compare
} | ||
} | ||
|
||
fn get_trie_stats_rec<T: PartialTrie>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returning the state rather than using a mut state as an arg would be more rusty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only downside is then we would need to do more computation when merging together multiple CurrTrackingState
together, which I guess doesn't really matter too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will revisit this afterwards!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given it's just a debug tool I'm not too worried about the bits I noted. Having working tools is more useful than not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @BGluth!
I'll stop commenting now because most of my comments are identical. So global points:
- we can probably not have
pub
most methods / objects lying around. I think that's probably good practice to reduce public API in general. - we can add some clearer documentation on new types and methods, but it's not blocking.
Yeah agreed. Meant to comment this. |
Hmm yeah. So I ended up making a lot more of the interface public at the last minute because I thought the callers might want to access the stat data without relying just on |
It's not really blocking, we can always restrict the API in the future. This isn't even on crates.io yet, is it? |
It's on |
Co-authored-by: Robin Salen <[email protected]>
Ah nevermind for crates.io, for some reason I thought this was eth-trie-tools 🤦🏼♂️ |
A bit more tooling related to extracting stats from tries. Primary use case that I want from this is to compare sub-tries from the base trie they were created from.
To give an idea of what the output looks like: