-
Notifications
You must be signed in to change notification settings - Fork 145
[Testing Only] cacheless hamt iteration #2215
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
base: master
Are you sure you want to change the base?
[Testing Only] cacheless hamt iteration #2215
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2215 +/- ##
==========================================
+ Coverage 77.56% 77.61% +0.04%
==========================================
Files 147 147
Lines 15789 15872 +83
==========================================
+ Hits 12247 12319 +72
- Misses 3542 3553 +11
🚀 New features to boost your workflow:
|
494060d
to
dad03d3
Compare
@@ -1,7 +1,7 @@ | |||
[package] | |||
name = "fvm_ipld_hamt" | |||
description = "Sharded IPLD HashMap implementation." | |||
version = "0.10.4" | |||
version = "0.11.0" |
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.
do this as a separate PR
"bulk insert" - how big are we talking here? if you make it huge you ought to see a larger difference in the iteration I think, although this really does come down to memory retention and management, so measuring that would be more interesting if that were at all possible |
it'd be better if we didn't go breaking the existing API, can you not easily do a similar approach as in #2189 of adding a new iteration method for this? |
Hey @rvagg Thanks for your quick review! I will use another PR to implement I'd like to discuss whether we plan to adopt this PR in the next minor version release and deprecate |
It might not be beneficial in the current Forest logic, but some users might depend on the caching for their use cases, so this is a breaking change. Refer to Hyrum's Law. We might think about switching the backend to cacheless one eventually, but we'd need to ensure the benefits significantly outweigh potential risks. I'm especially concerned about potential performance degradation in builtin-actors on mainnet (but other consumers of the |
This is going to have to be case-by-case, which is why it would be nice to have an opt-in form of this. I did originally imagine having some constructor option that would give you one that cached or didn't cache, but this is much more a question of idiomatic Rust and Rust ergonomics which I'm not necessarily the best person to give advise on. But I do want to do this for Go to, and, I know its utility is mainly in certain areas, particularly where we know we're doing iteration on large data and we know that we're only iterating. Migrations is the big one for Go at least, we iterate through actors, we don't need random access, we drop it on the floor when we're done, the cache just gets in the way. I'm sure there's a number of APIs we serve too where it's pure straight iteration too where it would be helpful. But in the case where you instantiate one of these things and pass it around for general use - get, set, iterate, then a cache could be quite helpful. |
Benchmarks