core/vm: Refactor jump table to merge dynamic gas cost with memory expansion cost calculations #24113
Conversation
| @@ -55,45 +55,62 @@ func memoryGasCost(mem *Memory, newMemSize uint64) (uint64, error) { | |||
| return 0, nil | |||
| } | |||
|
|
|||
| func memoryGasCostAndSize(stack *Stack, mem *Memory, sizeFunc memorySizeFunc) (uint64, uint64, error) { | |||
There was a problem hiding this comment.
| func memoryGasCostAndSize(stack *Stack, mem *Memory, sizeFunc memorySizeFunc) (uint64, uint64, error) { | |
| func memoryGasCostAndSize(stack *Stack, mem *Memory, sizeFunc memorySizeFunc) (gas, memorySize uint64, err error) { |
You can name results (although it is still confusing for me that now these variables are defined from the beginning of the function).
There was a problem hiding this comment.
Thanks, I forgot about that, but not sure if I like this variant more, and whether it aligns well with the usual geth style.
func memoryGasCostAndSize(stack *Stack, mem *Memory, sizeFunc memorySizeFunc) (gas uint64, memorySize uint64, err error) {
memSize, overflow := sizeFunc(stack)
if overflow {
return 0, 0, ErrGasUintOverflow
}
// memory is expanded in words of 32 bytes. Gas is also calculated in words.
memorySize, overflow = math.SafeMul(toWordSize(memSize), 32)
if overflow {
return 0, 0, ErrGasUintOverflow
}
gas, err = memoryGasCost(mem, memorySize)
if err != nil {
return 0, 0, err
}
return
}
gumb0
force-pushed
the
dynamic-gas-refactor
branch
from
2ca57c6
to
048e8ea
Compare
|
As for usual geth-style, named returns are ok, but silent/implicit returns are no-no
|
|
Not much changes in my benchmarks: |
| func gasCallCode(evm *EVM, contract *Contract, stack *Stack, mem *Memory, memorySize uint64) (uint64, error) { | ||
| memoryGas, err := memoryGasCost(mem, memorySize) | ||
| func gasCallCode(evm *EVM, contract *Contract, stack *Stack, mem *Memory) (uint64, uint64, error) { | ||
| memoryGas, memorySize, err := memoryGasCostAndSize(stack, mem, memoryCall) |
There was a problem hiding this comment.
CALLCODE uses the same memory function as CALL
go-ethereum/core/vm/jump_table.go
Lines 1003 to 1009 in bc6bf1e
gumb0
force-pushed
the
dynamic-gas-refactor
branch
from
048e8ea
to
27887a4
Compare
gumb0
requested review from
holiman,
karalabe and
rjl493456442
as code owners
|
As it is, I'm a bit torn on this. It is a pretty deep change, which needs to be very thorougly reviewed, but the benchmarks do not really support the change. Now that you placed into dynamicGas, it might be possible to make the call to the memory calculation become inlined. But it might mean that instead of calling |
Merge dynamic gas cost calculation with memory expansion cost calculation.
chfast
force-pushed
the
dynamic-gas-refactor
branch
from
27887a4
to
cca2554
Compare
In this refactoring I keep the memory gas calculation general, by having low-level |
|
I don't see any clear improvement (historically the signextend is sensitive to any changes). |
|
I tried inlining |
|
I'll close this. Feel free to reopen if you manage to unlock some new level :) |
This was discussed in #24048 (comment)
Basically it merges
operation.dynamicGascall withoperation.memorySizecall into singledynamicGascall returning both gas required and new memory size (or 0 if memory size doesn't change).It allows to simplify logic in interpreter loop and also to get rid of init-time validation to check that
operation.memory != niliffoperation.dynamicGas != nil(Because it's now expected fromdynamicGasfunction author to correctly cover both costs,operation.memorymember is removed)