core/vm: avoid memory expansion check for trivial ops #24048
Conversation
|
Simple loop benchmark against Last release against last release against this PR |
| if op == nil { | ||
| panic(fmt.Sprintf("op 0x%x is not set", i)) | ||
| } | ||
| // The interpreter has an assumption that if the memorySize function is |
There was a problem hiding this comment.
Considering they always go together, it would be better to merge these into single function that outputs new memory size and error.
There was a problem hiding this comment.
Nitpick: they don't "always go together". If dynamic memory, then dynamic gas, but not always the other way around. For example the EXP or SSTORE and SELFDESTRUCT doesn't expand memory but have a dynamic cost.
But yes, maybe something could be changed in the strucutre at a higher level.
There was a problem hiding this comment.
Yes, what I meant in this case they should return unchanged memory size or 0.
But in total this reduces helper calls from 2 to 1 and eliminates the need for validate().
There was a problem hiding this comment.
eliminates the need for validate().
Afaik, the validate is only performed during geth bootup, when the global vars are instantiated. Unless I misunderstood something.
There was a problem hiding this comment.
I agree that it can be reworked, but I don't want to do it in this PR. I think merging memory funcs and gas funcs is going to be a very large change.
There was a problem hiding this comment.
Afaik, the validate is only performed during geth bootup, when the global vars are instantiated. Unless I misunderstood something.
Correct. But my comment was about the design. Not needing validate() is better design.
I agree that it can be reworked, but I don't want to do it in this PR.
Make sense.
There was a problem hiding this comment.
I am doing this refactoring in #24113
|
holiman
force-pushed
the
evm_fixmemcheck
branch
from
dbfc802
to
bc5dc7b
Compare
| // 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] | ||
| cost = operation.constantGas // For tracing |
There was a problem hiding this comment.
Yesterday I started a branch to experiment with moving all the gas calculation into another function (next to the implementation): https://github.com/ipsilon/go-ethereum/tree/gas-redesign
The constant gas instructions would have the same simple function charging the constant gas, others would have the dynamic calculation + memory expansion.
I am not sure if this will help everything considered, because it will always mean an indirect call, but at the same time it reduces most of the branching in this loop.
There was a problem hiding this comment.
Looked briefly at the last commit there -- I think it won't work that easily.
The way the dynamic gas has evolved, we need to first ensure that the caller can cover the static cost, because for some ops, actually calculating the dynamic cost is expensive: checking the existence of things in the trie.
So we can't just calculate everything and then charge.
We can get around it by either having the "chargeGas" function not only return the gas, but actually charge it via the contract.
Alternatively, we can pass the availableGas into the "chargeGas" function.
Cherry-pick ethereum/go-ethereum#24048 Co-authored-by: Martin Holst Swende <martin@swende.se>
This PR moves the dynamic memory check function to only be checked if the op has a dynamic gas function. This is strictly not a guarantee, just how things are, and it allows us to skip two conditionals on the cheap ops.
One minor semantic change is that
costis set earlier, making errors more correct than previously (if erroring on e.g. shallow stack)