Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hybrid EVM Stack Experiment #3029

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Hybrid EVM Stack Experiment #3029

wants to merge 14 commits into from

Conversation

holgerd77
Copy link
Member

This is a pretty extensive experiment switching the current BigInt based EVM stack with a hybrid stack which stores either the Bytes or the BigInt values and - if matches - serves the respetive values without conversion.

Results are unfortunately a lot more mixed than I anticipated. One opcode significantly increases in performance (MLOAD, going from 2.9 to 4.4 or so), but other values are mixed throughout the range, some somewhat faster, some somewhat slower.

Might come back to this with some distance, atm I am bit baffled that there is "not more to see".

Here is e.g. a resultset on the same block (left is with the hybrid stack).

grafik

My initial idea was to keep the old stack, but introduce a flag which would allow to switch to the new one. If results remain this mixed this might be obsolete though.

This work might nevertheless be useful for some things, e.g. it is now extremely easily possible to switch e.g. arithmetic operations from being done bigint based to byte based and see how performance reacts.

@holgerd77
Copy link
Member Author

  • This stack implementation is significantly more performant than the single type stack. Atm it is however
  • not activated by default for backwards compatibility reasons.

I was so confident in the outcome here that I had even already written class documentation before I had first results mentioning the increased performance. 😆

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #3029 (e71f171) into master (67a20de) will increase coverage by 3.39%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 88.73% <ø> (ø)
blockchain 92.58% <ø> (ø)
client 87.47% <ø> (+0.04%) ⬆️
common 98.18% <ø> (ø)
ethash ∅ <ø> (∅)
evm ?
rlp ∅ <ø> (∅)
statemanager 89.91% <ø> (ø)
trie 90.18% <ø> (ø)
tx 96.35% <ø> (ø)
util 86.78% <ø> (ø)
vm ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a lot of deep insight into this yet but left a couple of initial comments. It's an interesting idea for sure.

if (elem[0] !== undefined) {
return elem[0]
}
return bytesToBigInt(elem[1]!)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this scenario ever happen? Since the stack is always used internally and never exposed directly, can we safely assume if popBigInt is called that elem[1] will alaywas be undefined?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't be undefined if the type of the stack item is of type Uint8Array

if (elem[1] !== undefined) {
return elem[1]
}
return bigIntToBytes(elem[0]!)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above. Shouldn't we just return elem[1] here and leave it up to the consumer to know that it's bytes vs bigint?

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great experiment.

What we could also try is instead of using the current list of [bigint, Uint8Array] instead use two lists..? Not sure if that is more performant?

In general, can we also see if we can optimize bytesToBigInt / bigIntToBytes? We shuffle these values around a lot in our libs and if we can optimize those that would also be great.

* not activated by default for backwards compatibility reasons.
*
* In the current EVM version this stack performs significantly worse than the hybrid type stack on the EVM
* `step` event (if used), since there is mapping to a single type stack needed on each step.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. We might want to note that if one listens to step then EVM performance will be much slower due to the objects it has to generate. step event should be somewhat seen as a debug mode event.

if (elem[0] !== undefined) {
return elem[0]
}
return bytesToBigInt(elem[1]!)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't be undefined if the type of the stack item is of type Uint8Array

if (elem[1] !== undefined) {
return elem[1]
}
return bigIntToBytes(elem[0]!)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can optimize here: first get bigIntToBytes, then set elem[1] to that value, and then return the actual value. Same for popBigInt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, never mind, the values are popped so those are unreachable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants