Optimization: Small changes on many functions V3 (wip) #217
Conversation
This file contains 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
added 10 commits
August 28, 2024 11:22
|
Ergs comparison results: |
JulianVentura
commented
Aug 29, 2024
src/op_handlers/far_call.rs
Outdated
| // Invalid | ||
| _ => return Err(EraVmError::IncorrectBytecodeFormat), | ||
| }; | ||
|
|
||
| code_info_bytes[1] = 0; | ||
| // let code_key = U256::from_big_endian(&code_info_bytes); |
There was a problem hiding this comment.
Delete this comment
Oppen
reviewed
Aug 29, 2024
Comment on lines
-55
to
-59
| let mut address = 0; | ||
| for value in code.iter() { | ||
| heap.store(address, *value); | ||
| address += 32; | ||
| } |
There was a problem hiding this comment.
Seeing it starts with address 0, maybe we can just assign code to the new heap instead. This way we avoid all the copying and just move the whole vector.
There was a problem hiding this comment.
I actually don't know if there is an easy way of doing that, since code is of type Vec<U256> but heap is of type Vec<u8>.
I'll keep the code as is, but if you happen to know how to do that transformation it could be a good optimization.
There was a problem hiding this comment.
I am not saying to make this change now, but something to consider for a future pr: we could easily retrieve the code as a Vec<u8> by adding a new function to the Storage trait that decommits and returns it in that format. The reason it's easy is that in the current zksync-era storage, bytecodes are originally stored as Vec<u8>, and we are actually converting them to Vec (see here).
There was a problem hiding this comment.
Yep, if we are able to modify that trait (we are not following any API), then thats totally an option.
Oppen
reviewed
Aug 29, 2024
MarcosNicolau
approved these changes
Aug 30, 2024
Comment on lines
-55
to
-59
| let mut address = 0; | ||
| for value in code.iter() { | ||
| heap.store(address, *value); | ||
| address += 32; | ||
| } |
There was a problem hiding this comment.
I am not saying to make this change now, but something to consider for a future pr: we could easily retrieve the code as a Vec<u8> by adding a new function to the Storage trait that decommits and returns it in that format. The reason it's easy is that in the current zksync-era storage, bytecodes are originally stored as Vec<u8>, and we are actually converting them to Vec (see here).
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.
Description
This PR improves vm performance by making small changes to many functions.
Results
The results vary across different benchmark suites, showing changes from no difference to a 12% enhancement. When we aggregate the average execution times of all benchmarks, we observe an overall improvement of approximately 3%.
Dependencies
Changes were made to
default_aa_code_hashandevm_interpreter_code_hashvariable types underexecution.rswhich require a bump on underlyingzksync-erarepository dependency. Because of that, this PR should only be merged together withera_vm_small_changes_v2