core/vm: optimize opCreate and opCreate2 by not copy memory

[cuiweixie]

goos: darwin
goarch: arm64
pkg: github.com/ethereum/go-ethereum/core/vm/runtime
cpu: Apple M1 Pro
                    │   old.txt    │               new.txt               │
                    │    sec/op    │   sec/op     vs base                │
EVM_CREATE_500-10     16.285m ± 2%   1.701m ± 1%  -89.55% (p=0.000 n=10)
EVM_CREATE2_500-10     97.04m ± 0%   93.26m ± 1%   -3.90% (p=0.000 n=10)
EVM_CREATE_1200-10    22.029m ± 1%   1.338m ± 1%  -93.93% (p=0.000 n=10)
EVM_CREATE2_1200-10    82.56m ± 0%   80.26m ± 1%   -2.78% (p=0.001 n=10)
geomean                41.17m        11.43m       -72.25%

                    │    old.txt     │               new.txt                │
                    │      B/op      │     B/op      vs base                │
EVM_CREATE_500-10     144.244Mi ± 0%   1.091Mi ± 0%  -99.24% (p=0.000 n=10)
EVM_CREATE2_500-10    37926.2Ki ± 0%   660.1Ki ± 0%  -98.26% (p=0.000 n=10)
EVM_CREATE_1200-10    256.900Mi ± 0%   1.606Mi ± 0%  -99.37% (p=0.000 n=10)
EVM_CREATE2_1200-10    32.245Mi ± 0%   1.208Mi ± 0%  -96.25% (p=0.000 n=10)
geomean                 81.56Mi        1.081Mi       -98.67%

                    │   old.txt   │              new.txt               │
                    │  allocs/op  │  allocs/op   vs base               │
EVM_CREATE_500-10     15.28k ± 0%   14.52k ± 0%  -4.93% (p=0.000 n=10)
EVM_CREATE2_500-10    3.888k ± 0%   3.700k ± 0%  -4.82% (p=0.000 n=10)
EVM_CREATE_1200-10    11.72k ± 0%   10.90k ± 0%  -6.94% (p=0.000 n=10)
EVM_CREATE2_1200-10   1.425k ± 0%   1.348k ± 0%  -5.44% (p=0.000 n=10)
geomean               5.612k        5.301k       -5.54%

[jwasinger]

It seems like it could be okay. But also could potentially be a very dangerous change.

contract creation happens relatively less often and isn't a bottleneck in the EVM. So based on the risk/reward tradeoff here, I'd be inclined to close this.

[cuiweixie]

1. The OP_CREATE and OP_CREATE2 operations are similar to OP_CALL. I noticed that OP_CALL was updated from using a copy to using a pointer in this commit.
2. In both OP_CREATE and OP_CREATE2, the init code is used as the contract code. Contract code is immutable by consensus in the blockchain community. During the execution of OP_CREATE/OP_CREATE2, new mem obj will be create in this code, and opReturn already uses memory.GetCopy. Therefore, there should be no potential risk in this case.

[gballet]

I have digged into this, and even though this thing looks wrong, I am convinced that it is ok:

• the input data would be a reference, but accessing it from the contract would cause a copy as CALLDATALOAD calls getData, which makes a copy. There is no way for the caller to directly modify the input slice.
• the data that would be saved to disk is also copied from memory, so there is no issue there
• As the author notes, this optimization is already used in opCall and it works

Nonetheless, I think it's a bad idea because although it works today, we are not protected from future modifications that would somehow uncover that problem. I'm going to approve but let Gary have the final say.

[rjl493456442]

I agree with Guillaume, and I prefer not to accept it, for the following reasons:

The benefit is negligible. CREATE and CREATE2 are far less common than CALL*, so the allocation savings are typically insignificant at the overall level.

Although the contract code is currently treated as an immutable variable (since during CALLDATACOPY, the data is deep-copied into memory), there's no guarantee that this assumption will hold in the future. For example, in Verkle, we may need to chunkify the code.

Compare with the limited gain and the potential risk, i don't think it's worthwhile to have this change.

[cuiweixie]

I agree that this is dangerous. may cause some thing like https://github.com/ethereum/go-ethereum/blob/6f2cbb7a27ba7e62b0bdb2090755ef0d271714be/docs/postmortems/2021-08-22-split-postmortem.md