Use Map for OpcodeList and opcode handlers #852
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
ryanio
force-pushed
the
use-maps-for-opcodes
branch
from
f65a4db
to
e89672f
Compare
|
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. |
|
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
What is this change about?
There was a problem hiding this comment.
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) |
ryanio
force-pushed
the
use-maps-for-opcodes
branch
from
bb1cc40
to
ad4625e
Compare
|
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. |
This PR targets resolving the following from #775: