-
Notifications
You must be signed in to change notification settings - Fork 785
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
Statemanager: Add Separate Cache-Free StateManager version for Client Execution / copy() Options Fix #2582
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
…y() not taking options into account
a1e6fc3
to
d3b4acd
Compare
Update: I know settled with a new I think this is still sufficiently clean enough and all current SM functionality/extensibility should be preserved. It might eventually be nice if we are able to abstract the cache generally a bit more out and decouple along the next breaking releases, just as some additional general note. This should be open for review now. Together with #2581 this now allows to sync on mainnet past the first round of problematic Shanghai dDoS blocks or re-run with: npm run client:start -- --discDns=false --discV4=false --maxPeers=0 --executeBlocks=2283416 🙂 |
Side note: when looking at this with a step back I have to say that this new option generally makes sense to allow for easier cache/no-cache run comparisons, generally people might want to switch off cache for whatever reasons. Also to note, forgot to mention: this PR also fixes a StateManager.copy() bug not copying the constructor option settings along. |
Some additional note on this after some more reflection: I still DO think that the functionality here (to have the possibility to deactivate SM cache) is useful and we should merge this. This does not however mean that we need to live without the SM cache for now on forewards. At the moment it seems the better option for the client. We can still easily experiment with the cache code we have though and eventually re-activate. I was just thinking that in fact it might also already work if we track (within the SM cache) if there are cache changes during a CP period with a flag, and only copy over the whole cache in the Absolutely no hurry here but we can just test this out in the coming days as well. Eventually this will already solve a lot of (all?) practical out-of-memory EVM code run situations. |
4f58424
to
a5fb9be
Compare
Closed in favor of #2592 |
Part of #1536
Extracted (variant) from #2578
Ugh.
This is a hard one. 🙂
During the last couple of days I experimented a lot with the StateManager cache. The cache also has the problem of being very inefficient and memory-bloating, by just copying over the whole cache during a
commit()
.In fact this is the substantially more important cache (inefficiency) since this cache is a lot larger than the one being optimized in #2581 (touched accounts in VM). I did two substantial rewrites of this cache transforming to a diff-based structure, each taking me several hours and I both made it to the point that blocks from mainnet can be executed subsequently. The first can be seen here (+ the follow-up commit) which uses a somewhat more complex structure for account entries containing not the values itself but an OrderedMap associating checkpoint heights with respective changes on the account. I then did another version of this with a simpler native datastructure (assuming better performance), taking just an array containing the CP height and cache entry somewhat like this `[ CPHeight (e.g. 3), CacheEntry (the old {val, modified,... } ].
To make it short: at the end it turned out that the by far fastest/most performant version was to use no cache at all respectively eliminate. 😂 (for the rescue: the diff based caches at least solved (like the no cache version), the memory crash).
The non-cache version is superior on the problematic blocks by a factor of 4-10 (!) (14 sec vs 2+ min for the memory crash block e.g.) and has no noticeable downturns on "normal" block executions.
Ugh. Mumble. 😋
This took me a while to let this sink in. After a couple of days of having let this sink in I would then say: ok. If this is a viable solution to make things better then let's remove, at least for the client.
To not side-affect existing users by this I would then just create a new
CachelessStatemanager
separate to the normal default state manager, which we can separately use for the client (and eventually also collect some more experiences with this on our own use case) and we can then see what we will take from this and how to proceed along our breaking releases.Additionally we can also label this new
CachelessStatemanager
asexperimental
if we want to discourage usage for now.WIP, need to clean things up, will drop a note once ready. 🙂