perf: refactor interpreter internals#558
Closed
DaniPopes wants to merge 13 commits intobluealloy:mainfrom
Closed
Conversation
2dd9e0c to
3658fa7
Compare
DaniPopes
commented
Jul 25, 2023
Collaborator
Author
DaniPopes
left a comment
There was a problem hiding this comment.
General changes:
- restrict visibility of items that are not exported to emphasize that they are private
- omit error from
as_usize_or_fail!macro calls when it's the default (InvalidOperandOOG) - rewrote benches from
revm-testto criterion, and moved them tocrates/revm/benches/
Collaborator
Author
There was a problem hiding this comment.
Changes:
- cmp opcodes are now branchless
byteindexes into the slice instead of performing shiftssarusesU256::MAXinstead of computing it at runtime
Collaborator
Author
There was a problem hiding this comment.
Changes:
Displayshould not be gated tostd- avoid using manual implementations (like swap and pop) where possible by deferring to the standard library. This produces the same instructions while being safer
Collaborator
Author
There was a problem hiding this comment.
Changes:
- declare opcode constants, names (
OPCODE_JUMPMAP) and theevalfunction in one macro call
66c08a0 to
c771cc1
Compare
DaniPopes
commented
Jul 26, 2023
Collaborator
Author
There was a problem hiding this comment.
Changes:
- change
is_none()+unwrapto uselet else
1a0b3d3 to
b937b87
Compare
b937b87 to
d70c87b
Compare
`push_b256!` will convert the big-endian `[u8; 32]` to the U256 `[u64; 4]` at runtime, so use `push!` to avoid doing that where possible.
f6f3fba to
d89e56d
Compare
d89e56d to
c657758
Compare
4f5e0ba to
2a06916
Compare
rakita
requested changes
Jul 31, 2023
Member
There was a problem hiding this comment.
@DaniPopes you made too many changes here to be accepted as it is, and I don't agree with some of them
2a06916 to
c536272
Compare
Collaborator
Author
|
@rakita Sure makes sense, what do you want me to split off / change? |
Collaborator
Author
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
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.
This PR focuses on refactoring some parts of the interpreter which lead to, although minor, performance gains.
Closes #310, not fixes, as the current
matchperforms better on debug builds, and performs the same in release builds. This is not shown in the results.Results
Cachegrind
See #582
Benches
Not entirely sure why the non-generic version performs better, since I would expect the checks to be optimized out at compile time on the generic version.I have included the patch which can be used to change between the two versions.Time benches shouldn't be relied upon for these type of small changes, and these ones are also outdated.
Main
36de35b
Generic (patch)
3658fa7
Patch
Non generic
3658fa7