refactor(levm): implement cache rollback#2417
Merged
Merged
Conversation
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
|
JulianVentura
left a comment
Contributor
There was a problem hiding this comment.
Code looks good, just two comments
|
|
||
| /// 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. |
Contributor
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
| pub fn get_account_mut<'a>( | ||
| &'a mut self, | ||
| address: Address, | ||
| call_frame: Option<&mut CallFrame>, |
Contributor
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)?;**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 <43799596+JulianVentura@users.noreply.github.com> Co-authored-by: Avila Gastón <72628438+avilagaston9@users.noreply.github.com> Co-authored-by: fmoletta <99273364+fmoletta@users.noreply.github.com> Co-authored-by: Matías Onorato <onoratomatias@gmail.com> Co-authored-by: Edgar <git@edgl.dev> Co-authored-by: Tomás Arjovsky <tomas.arjovsky@lambdaclass.com> Co-authored-by: Martin Paulucci <martin.c.paulucci@gmail.com> Co-authored-by: Lucas Fiegl <iovoid@users.noreply.github.com> Co-authored-by: Estéfano Bargas <estefano.bargas@fing.edu.uy> Co-authored-by: Javier Rodríguez Chatruc <49622509+jrchatruc@users.noreply.github.com> Co-authored-by: VolodymyrBg <aqdrgg19@gmail.com> Co-authored-by: Tomás Paradelo <112426153+tomip01@users.noreply.github.com> Co-authored-by: Mauro Toscano <12560266+MauroToscano@users.noreply.github.com> Co-authored-by: Cypher Pepe <125112044+cypherpepe@users.noreply.github.com>
This was referenced Apr 24, 2025
…nto levm/cache_rollback
JulianVentura
approved these changes
Apr 25, 2025
JulianVentura
left a comment
Contributor
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> { |
Contributor
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> { |
Contributor
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) { |
Contributor
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.
Contributor
Author
|
This PR has become too big so the best option is to merge it and address the comments later :) |
pedrobergamini
pushed a commit
to pedrobergamini/ethrex
that referenced
this pull request
Aug 24, 2025
**Motivation** <!-- Why does this pull request exist? What are its goals? --> - Implement cache rollback for avoiding cloning the cache during the execution of a transaction. **Description** - Now callframe has `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. - Move some database related functions that don't need backup to `GeneralizedDatabase` - Move some database related functions that need backup `VM`. Basically it accesses the database backup up if there's a callframe available for doing so. - Stop popping callframe whenever possible Some other changes that it makes: - Simplify `finalize_execution`. Specifically the reversion of value transfer and removal of check for coinbase transfer of gas fee. - Move some things to `utils.rs` and `gen_db.rs` so that `vm.rs` keeps main functionalities. Closes #issue_number --------- Co-authored-by: Javier Chatruc <jrchatruc@gmail.com> Co-authored-by: Tomás Paradelo <tomas.paradelo@lambdaclass.com> Co-authored-by: Julian Ventura <43799596+JulianVentura@users.noreply.github.com> Co-authored-by: Avila Gastón <72628438+avilagaston9@users.noreply.github.com> Co-authored-by: fmoletta <99273364+fmoletta@users.noreply.github.com> Co-authored-by: Matías Onorato <onoratomatias@gmail.com> Co-authored-by: Edgar <git@edgl.dev> Co-authored-by: Tomás Arjovsky <tomas.arjovsky@lambdaclass.com> Co-authored-by: Martin Paulucci <martin.c.paulucci@gmail.com> Co-authored-by: Lucas Fiegl <iovoid@users.noreply.github.com> Co-authored-by: Estéfano Bargas <estefano.bargas@fing.edu.uy> Co-authored-by: Javier Rodríguez Chatruc <49622509+jrchatruc@users.noreply.github.com> Co-authored-by: VolodymyrBg <aqdrgg19@gmail.com> Co-authored-by: Tomás Paradelo <112426153+tomip01@users.noreply.github.com> Co-authored-by: Mauro Toscano <12560266+MauroToscano@users.noreply.github.com> Co-authored-by: Cypher Pepe <125112044+cypherpepe@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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