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

Use Map for OpcodeList and opcode handlers #852

Merged
merged 3 commits into from
Sep 14, 2020
Merged

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Aug 28, 2020

This PR targets resolving the following from #775:

  • The opcodes handlers map is indexed by string, it should be done by number.
  • The opcodes handlers map and the opcodes OpcodeList's one, should probably be Maps. The semantic of Map is way simpler than an accessing an object with object[key].

@codecov
Copy link

codecov bot commented Aug 28, 2020

Codecov Report

Merging #852 into master will increase coverage by 0.10%.
The diff coverage is 96.40%.

Impacted file tree graph

Flag Coverage Δ
#account 92.85% <ø> (ø)
#block 77.37% <ø> (-0.04%) ⬇️
#blockchain 81.10% <ø> (+0.30%) ⬆️
#common 94.73% <ø> (+0.15%) ⬆️
#ethash 83.33% <ø> (+1.11%) ⬆️
#tx 93.98% <ø> (-0.14%) ⬇️
#vm 82.32% <96.40%> (+0.07%) ⬆️

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

@alcuadrado
Copy link
Member

I have another idea of how this can be optimized. I built a small script simulating the current implementation, and this other idea, but somehow lost it 😅

I spent some time reading about how to create an efficient jump table in js, and apparently the best way of doing it is with an array. Something important is that we shouldn't leave empty indexes. Instead, an invalid/missing-opcode should be used. This way v8 will, hopefully, optimize it.

Another important thing can be to stop creating opcodes per hardfork, and put the HF validation logic inside each opcode. This way we can use a static array of ocpdes, increasing the chances of this getting optimized.

@jochem-brouwer
Copy link
Member

I don't think we should go back to the point where we validate if an Op can be ran inside the Op itself. We can create multiple static arrays, where each array would be the array which holds the Ops of a certain HF. While checking HFs should not take a lot of time, I don't think we should check those at runtime but do that in the initialization phase.

evertonfraga
evertonfraga previously approved these changes Sep 11, 2020
Copy link
Contributor

@evertonfraga evertonfraga left a comment

Choose a reason for hiding this comment

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

That's great!

Now, I guess the only thing is to decide along with @alcuadrado what route to take wrt Map vs. Arrays.

} else {
r = new BN(0)
r = c
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this change about?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! got it. After a more thorough review, I found that the diff has drifted a some lines. All good


return opcode
// if not found, return 0xfe: INVALID
return this._vm._opcodes.get(op) || this._vm._opcodes.get(0xfe)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@ryanio ryanio force-pushed the use-maps-for-opcodes branch from bb1cc40 to ad4625e Compare September 12, 2020 01:07
@ryanio
Copy link
Contributor Author

ryanio commented Sep 12, 2020

thanks for the review @evertonfraga :) @alcuadrado said we can merge this for now since it's already an improvement, and do a follow-up PR for the array optimization.

i just had to resolve a merge conflict, but should be ready for a final approve and merge now.

@evertonfraga evertonfraga self-requested a review September 14, 2020 18:52
@ryanio ryanio merged commit 4703d10 into master Sep 14, 2020
@evertonfraga evertonfraga deleted the use-maps-for-opcodes branch September 14, 2020 19:01
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.

4 participants