refactor(levm): implement cache rollback #2417
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lines of code reportTotal lines added: Detailed view |
Benchmark Results ComparisonPR ResultsBenchmark Results: Factorial
Benchmark Results: Factorial - Recursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: BubbleSort
Benchmark Results: ERC20 - Transfer
Benchmark Results: ERC20 - Mint
Benchmark Results: ERC20 - Approval
Main ResultsBenchmark Results: Factorial
Benchmark Results: Factorial - Recursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: BubbleSort
Benchmark Results: ERC20 - Transfer
Benchmark Results: ERC20 - Mint
Benchmark Results: ERC20 - Approval
|
EF Tests Comparison
|
crates/vm/levm/src/db/gen_db.rs
Outdated
|
|
||
| /// Gets mutable account, first checking the cache and then the database | ||
| /// (caching in the second case) | ||
| /// This isn't a method of VM because it allows us to use it during VM initialization. |
There was a problem hiding this comment.
I don't think this line comment adds much to this method documentation. Having this method on the GeneralizedDatabase makes more sense than having it on the VM
crates/vm/levm/src/db/gen_db.rs
Outdated
| pub fn get_account_mut<'a>( | ||
| &'a mut self, | ||
| address: Address, | ||
| call_frame: Option<&mut CallFrame>, |
There was a problem hiding this comment.
I think that it is not super clear here why you are passing an Option<CallFrame>.
If you want to make the cache backup optional, then maybe we need something a bit more expressive.
The best I came up with right now is to have
type CacheBackup = HashMap<Address, Option<Account>>;
pub fn get_account_mut<'a>(
&'a mut self,
address: Address,
cache_backup: Option<&mut CacheBackup>);
// The call would be
self.db.get_account_mut(address, Some(call_frame.previous_cache_state))?;
self.db.get_account_mut(address, None)?;There was a problem hiding this comment.
Forgot to say, I changed it here
**Motivation** <!-- Why does this pull request exist? What are its goals? --> - merge main changes to my other PR because main had a huge refactor and my PR was also a huge refactor :) **Description** <!-- A clear and concise general description of the changes this PR introduces --> <!-- Link to issues: Resolves #111, Resolves #222 --> Closes #issue_number --------- Co-authored-by: Julian Ventura <[email protected]> Co-authored-by: Avila Gastón <[email protected]> Co-authored-by: fmoletta <[email protected]> Co-authored-by: Matías Onorato <[email protected]> Co-authored-by: Edgar <[email protected]> Co-authored-by: Tomás Arjovsky <[email protected]> Co-authored-by: Martin Paulucci <[email protected]> Co-authored-by: Lucas Fiegl <[email protected]> Co-authored-by: Estéfano Bargas <[email protected]> Co-authored-by: Javier Rodríguez Chatruc <[email protected]> Co-authored-by: VolodymyrBg <[email protected]> Co-authored-by: Tomás Paradelo <[email protected]> Co-authored-by: Mauro Toscano <[email protected]> Co-authored-by: Cypher Pepe <[email protected]>
This was referenced Apr 24, 2025
…nto levm/cache_rollback
JulianVentura
approved these changes
Apr 25, 2025
There was a problem hiding this comment.
Left you some nit comments. We should take a look at the clones, since the recent flamegraph that we made while syning in holesky showed us that we spend too much time cloning data from hashmaps and Account.
Anyways, the code LGTM and it has shown some performance improvements while syncing, so good work!
We can merge and try to remove some data cloning in other PR
| // ================== Account related functions ===================== | ||
| /// Gets account, first checking the cache and then the database | ||
| /// (caching in the second case) | ||
| pub fn get_account(&mut self, address: Address) -> Result<Account, DatabaseError> { |
There was a problem hiding this comment.
Can't we return a reference to the Account here, instead of cloning it? The caller may clone if it needs to.
Cloning too many accounts may be expensive, even more if those Accounts are contracts (contain bytecode and storage).
| } | ||
|
|
||
| /// Gets account without pushing it to the cache | ||
| pub fn get_account_no_push_cache(&self, address: Address) -> Result<Account, DatabaseError> { |
| &mut self, | ||
| accrued_substate: &mut Substate, | ||
| address: Address, | ||
| ) -> Result<(AccountInfo, bool), DatabaseError> { |
There was a problem hiding this comment.
Here we may be ok by cloning the AccountInfo, since it does not contain the bytecode nor storage
| // ================== Account related functions ===================== | ||
|
|
||
| pub fn get_account_mut(&mut self, address: Address) -> Result<&mut Account, VMError> { | ||
| if !cache::is_account_cached(&self.db.cache, &address) { |
There was a problem hiding this comment.
nit: Instead of interacting with the db.cache, why don't you make a function for the storage which retrieves what you want?
In this case, we could have a get_account_mut on the GeneralizedDatabase which returns a mutable reference to the Account.
You can then clone that if you want to store it in the backup.
|
This PR has become too big so the best option is to merge it and address the comments later :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Description
cache_backup, that stores the pre-write state of the account that the callframe is trying to mutate. If the context reverts that state is restored in the cache. Otherwise, the parent call frame inherits the changes of the child of the accounts that only the child has modified, so that if the parent callframe reverts it can revert what the child did.GeneralizedDatabaseVM. Basically it accesses the database backup up if there's a callframe available for doing so.Some other changes that it makes:
finalize_execution. Specifically the reversion of value transfer and removal of check for coinbase transfer of gas fee.utils.rsandgen_db.rsso thatvm.rskeeps main functionalities.Closes #issue_number