refactor(levm): remove clones for account structs#2684
Conversation
… clones and changed code where appropriate
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
|
SDartayet
added
performance
JereSalo
left a comment
JereSalo
left a comment
There was a problem hiding this comment.
The advantage of not cloning the Account when getting it comes from not cloning it's storage I think.
Maybe sometimes we are better off by doing let info = vm.db.get_account(sender_address)?.info.clone() rather than adding lines of code to make assignations to variables in some cases just because performance will probably not be affected from that clone and it is a little bit more concise.
I like the initiative of not cloning Account when doing get_account() but we need to find a way that is the simplest and adds the least amount of code I think. Maybe cloning AccountInfo if we need both nonce and balance is not a bad idea. Other ideas are welcome too for improving simplicity.
Let me know what you think. This PR looks good though. Good job :)
| // If the account already exists, we can just update its info in the array and avoid cloning it | ||
| if let Some(account) = self.accounts.get_mut(&update.address) { | ||
| account.nonce = info.nonce; | ||
| account.balance = info.balance; | ||
| account.code_hash = info.code_hash; | ||
| } else { | ||
| let account_info = AccountInfo { | ||
| nonce: info.nonce, | ||
| balance: info.balance, | ||
| code_hash: info.code_hash, | ||
| }; | ||
|
|
||
| //Update the account info | ||
| self.accounts.insert(update.address, account_info); | ||
| } |
There was a problem hiding this comment.
Honestly here I'd just do:
self.accounts.insert(update.address, info.clone());
It is way more simple and we are just cloning account info, which is pretty small. I think it is worth it for making the code simpler because the perfromance difference is negligible I'd say.
We should optimize for performance when cloning an entire Account that has storage in it. But these cases in which we clone AccountInfo are probably not significant. And we prefer simplicity in this scenario.
What do you think?
There was a problem hiding this comment.
Agree given that it should be pretty small and we want to go for simplicity. Also it will be better for isolating the changes, if we want to test the performance impact of removing the clones of AccountInfo we can do it in another branch
rodrigo-o
left a comment
rodrigo-o
left a comment
There was a problem hiding this comment.
Looking great, agreed with the comment regarding simplicity and the cloning of account info, we can create a small PR later with this if we still find it impacts performance.
| // If the account already exists, we can just update its info in the array and avoid cloning it | ||
| if let Some(account) = self.accounts.get_mut(&update.address) { | ||
| account.nonce = info.nonce; | ||
| account.balance = info.balance; | ||
| account.code_hash = info.code_hash; | ||
| } else { | ||
| let account_info = AccountInfo { | ||
| nonce: info.nonce, | ||
| balance: info.balance, | ||
| code_hash: info.code_hash, | ||
| }; | ||
|
|
||
| //Update the account info | ||
| self.accounts.insert(update.address, account_info); | ||
| } |
There was a problem hiding this comment.
Agree given that it should be pretty small and we want to go for simplicity. Also it will be better for isolating the changes, if we want to test the performance impact of removing the clones of AccountInfo we can do it in another branch
We are about to start testing the difference between main and this branch in syncing, we will be able to see the improvements of this PR and if they are significant as we think we can go either with a bit of finetuning regarding some of the clones for simplicity or create an issue for tackling it immediatly after in a follow-up. |
Performance Comparison: Syncing with HoleskyRun Details:
Specs:
Sync Performance:
Average batch time for 'this branch' is calculated from the provided subsequent batch times. Flamegraph Observations:
Main branch flamegraph, zoomed in to see the clone operations: This branch flamegraph: |
SDartayet
added
performance
Benchmark Block Execution Results Comparison Against Main
|
JereSalo
left a comment
JereSalo
left a comment
There was a problem hiding this comment.
good pr, left some suggestions and I think it will be ready to merge afterwards
**Motivation** Improving the performance of some cases by avoiding clones where possible. **Description** Many clones of account structs were removed. This involved changing the output of the get_account and access_account functions of the DB to return a reference to an account, as well as refactorings of the code which involved these functions. Resolves [lambdaclass#2611](lambdaclass#2611)
3 participants
Motivation
Improving the performance of some cases by avoiding clones where possible.
Description
Many clones of account structs were removed. This involved changing the output of the get_account and access_account functions of the DB to return a reference to an account, as well as refactorings of the code which involved these functions.
Resolves #2611