core/vm: avoid copying jumptable #21180
Conversation
| cfg.ExtraEips = append(cfg.ExtraEips[:i], cfg.ExtraEips[i+1:]...) | ||
| log.Error("EIP activation failed", "eip", eip, "error", err) | ||
| } | ||
| var jt *JumpTable |
There was a problem hiding this comment.
There is a nicer way to do this: if you change the type of jt to just JumpTable, you don't need to have all those &s below and you remove the extra copy in the loop below.
There was a problem hiding this comment.
Not sure if that defeats the optimization though :)
There was a problem hiding this comment.
I think that defeats the entire purpose of the PR... ?
There was a problem hiding this comment.
Hmm, but you do have to copy it when there are ExtraEips. The previous
version did multiple copies. If you apply my suggestion, there will be exactly
one copy. Can't really avoid that one if you want to be able to modify the instruction set before executing.
I'm really wondering about the performance impact of this change.
I think the idea is to speed up NewEVMInterpreter, but AFAIK that's not
really the bottleneck for anything.
There was a problem hiding this comment.
Anyway, you can disregard my comment. We should still measure what the impact of this is, can't be that much.
| @@ -225,7 +222,7 @@ func (in *EVMInterpreter) Run(contract *Contract, input []byte, readOnly bool) ( | |||
| // Get the operation from the jump table and validate the stack to ensure there are | |||
| // enough stack items available to perform the operation. | |||
| op = contract.GetOp(pc) | |||
| operation := in.cfg.JumpTable[op] | |||
| operation := &in.jt[op] | |||
There was a problem hiding this comment.
If there is any performance gain, it's likely from this change instead of the change in NewEVMInterpreter because this statement executes every instruction.
|
I don't think the goal was performance increase here, rather reducing
allocations.
…On Fri, Jun 5, 2020, 18:41 Felix Lange ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In core/vm/interpreter.go
<#21180 (comment)>
:
> @@ -225,7 +222,7 @@ func (in *EVMInterpreter) Run(contract *Contract, input []byte, readOnly bool) (
// Get the operation from the jump table and validate the stack to ensure there are
// enough stack items available to perform the operation.
op = contract.GetOp(pc)
- operation := in.cfg.JumpTable[op]
+ operation := &in.jt[op]
If there is any performance gain, it's likely from this change instead of
the change in NewEVMInterpreter because this statement executes every
instruction.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#21180 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA7UGMQHJBU7GNSCNSFZADRVEG4LANCNFSM4NTMCGQQ>
.
|
|
Extra eips only ever happen on state test execution, never in "real life"
|
|
I ran my SnailTracer benchmark against master vs. this PR, and there's a ~5% EVM number crunching speed improvement. It's not "amazing", but it's definitely noticeable. |
|
Though I don't see an allocation difference, so not sure if there's actually something better or if it's just benchmarking fluke |
|
I've played with this PR a bit and a few things I've learned:
All in all I think we can close this PR as it does not save any allocs whatsoever and seems to change code that's noop and open a new PR with the operation pointer optimization. |
|
This is my alternative PR that performs even better than this one #21336 |
credits: erigontech/erigon#564
ref: #21179