Preserve cache on reverting the snapshot #2488
Conversation
arkpar
added
A0-pleasereview 🤓
arkpar
added
A0-pleasereview 🤓
|
|
||
| // Replace data with another entry but preserve storage cache | ||
| fn merge_snapshot(&mut self, other: AccountEntry) { | ||
| self.state = other.state; |
There was a problem hiding this comment.
what if the new storage is dirty but the other account is clean?
There was a problem hiding this comment.
Its the other way round. other contains "new" changes that replace self. Everything in self is discarded except for the storage cache
There was a problem hiding this comment.
maybe rename other to newer, or override.
There was a problem hiding this comment.
also it's not really merge - it's more like overlay or replace.
There was a problem hiding this comment.
Aren't the items in the cache dirtier than those in the snapshots? In revert_snapshot it merges the snapshotted account into the cached, overwriting the cached account with the (cleaner) snapshotted account while preserving the dirty storage (which revert_snapshot should be throwing away?)
The name is a bit confusing too, it seems like it would be used in clear_snapshot but is in fact only used when reverting. (edit: looks like Gav got to this point first)
There was a problem hiding this comment.
@rphmeier That's correct. Dirty storage is discarded, but the clean storage cache is preserved
There was a problem hiding this comment.
Renamed into replace_with
There was a problem hiding this comment.
just to clarify, the purpose is basically to completely replace the cached account with the snapshotted block, while also preserving any new cached storage values since the snapshotted version was put away?
| @@ -394,7 +394,7 @@ impl Account { | |||
| /// Replace self with the data from other account merging storage cache. | |||
| /// Basic account data and all modifications are overwritten | |||
| /// with new values. | |||
| pub fn merge_with(&mut self, other: Account) { | |||
| pub fn replace_with(&mut self, other: Account) { | |||
There was a problem hiding this comment.
actually, having thought about it, i think overwrite_with is best.
| fn replace_with(&mut self, other: AccountEntry) { | ||
| self.state = other.state; | ||
| match other.account { | ||
| None => self.account = None, |
There was a problem hiding this comment.
more readable to keep Some and None in same order, at least locally?
gavofyork
added
A6-mustntgrumble 💦
|
minor grumbles; generally fine. |
arkpar
added
A0-pleasereview 🤓
|
Renamed a few methods and added some documentation on snapshotting. |
rphmeier
added
A8-looksgood 🦄
* Preserve cache on reverting the snapshot * Renamed merge_with into replace_with * Renamed and documented snapshotting methods
* RocksDB version bump * Preserve cache on reverting the snapshot (#2488) * Preserve cache on reverting the snapshot * Renamed merge_with into replace_with * Renamed and documented snapshotting methods * Track dirty accounts in the state (#2461) * State to track dirty accounts * Removed clone_for_snapshot * Renaming stuff * Documentation and other minor fixes * Replaced MaybeAccount with Option * Adjustable stack size for EVM (#2483) * stack size for io workers & evm threshold * rust way to remember stack size * right value * 24kb size * some stack reduction * Fixed overflow panic in handshake_panic (#2495)
No description provided.