EVM: Avoid memory.read() Memory Copy

[holgerd77]

This is a first cautious test to avoid copying memory buffers on memory.read() calls, realized by a new parameter avoidCopy which allows to directly return the memory slice in case there is no need for 0-filling.

First PR submit activates this for CREATE opcode to see what this will do to the tests, also generally direct read will likely have the biggest effect for these calling opcodes, since there potentially large chunks of data are read from memory.

[codecov]

Codecov Report

Merging #2573 (e0dce0c) into master (26a8dcd) will increase coverage by 1.73%.
The diff coverage is n/a.

Additional details and impacted files

Flag	Coverage Δ
block	90.13% <ø> (ø)
blockchain	90.40% <ø> (ø)
client	87.68% <ø> (ø)
common	95.82% <ø> (ø)
devp2p	91.67% <ø> (-0.09%)	⬇️
ethash	∅ <ø> (∅)
evm	?
rlp	∅ <ø> (?)
statemanager	89.61% <ø> (ø)
trie	90.36% <ø> (-0.30%)	⬇️
tx	94.16% <ø> (ø)
util	84.50% <ø> (ø)
vm	83.87% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[holgerd77]

For the record: these two client test cases are breaking on first commit CI run, not sure if related:

 [03-07|22:08:30] INFO Miner: Assembling block from 0 eligible txs (baseFee: 1000000000)
ok 65 should be strictly equal
not ok 66 baseFee should be as calculated
  ---
    operator: equal
    expected: 875000000n
    actual:   1000000000n
    at: Test.<anonymous> (/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/packages/client/test/miner/miner.spec.ts:543:7)
    stack: |-
      Error: baseFee should be as calculated
          at Test.assert [as _assert] (/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/node_modules/tape/lib/test.js:314:54)
          at Test.bound [as _assert] (/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/node_modules/tape/lib/test.js:99:32)
          at Test.strictEqual (/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/node_modules/tape/lib/test.js:478:10)
          at Test.bound (/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/node_modules/tape/lib/test.js:99:32)
          at Test.<anonymous> (/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/packages/client/test/miner/miner.spec.ts:543:7)
  ...
not ok 67 should be truthy

[holgerd77]

Test result on second commit (exanding to other calling opcodes) look worse than it is, mostly 1 additional test failing.

[holgerd77]

This actually still cuts test run times of the first 4 tests into half - just tested - so even in this limited scope and like this we should very much get this under control and we can eventually slowly expand (not sure what happens e.g. on the mload opcode, wouldn't want to include here to not overload). 🙂

[jochem-brouwer]

Ah OK great PR 😄 . I think with these complex fixes we should be extra cautious - we should temporarily disable the Homestead test which always fails so we also run that entire suite (it crashes at that Shanghai test since Node is out-of-memory).

[jochem-brouwer]

I think what we can do here is:

• Return the slice of the _store
• In case that loaded.length does not equal the size, expand the current internal _store with extra zeros. (Use the extend method to allocate in 8kb chunks)

[holgerd77]

A that's a good idea, I will try that (have got 3 work hours from now on). I would nevertheless want to keep this in this limited scope (so "free" the memory one after one or - rather - over time eventually), otherwise this will get too hairy and uncontrollable.

In the current scope this feels already pretty good though, will also test on some couple of hundred tx loaded mainnet blocks.

[holgerd77]

Direct update: ok, tested with npm run client:start -- --discDns=false --discV4=false --maxPeers=0 --executeBlocks=1400000-1401000 on a couple of thousand txs, that all went well, only one fork (homestead) but already a good sign. 🙂

[holgerd77]

Hmm. There is actually already a this.extend(offset, size) call at the beginning of the method.

I don't think it would have any effect if I just delete the zero filling conditional (not seeing how this can ever evaluate to true in the current setup of the method).

    if (loaded.length < size) {
      // fill the remaining part of the Buffer with zeros
      returnBuffer.fill(0, loaded.length, size)
    }

Will do that.

Or maybe, from a procedural point of view: will first look into the current test failures, and then do it. 😋

[jochem-brouwer]

Ah right, sorry, there is indeed the extend method above.

Somewhat unrelated, but each time we extend and it creates a 8kb memory, we concat the Buffer which, I think (https://nodejs.org/api/buffer.html#static-method-bufferconcatlist-totallength) copies the previous Buffer as well, which we also might not want. I am not sure how other clients allocate these Buffers, you could split them up in chunks of whatever size without copying them, but then if you want to read over 2+ containers the read-logic gets a big complex (and you have to copy them again). (Think of memory in this "optimized" (?) case as a linked list?)

[holgerd77]

Oh, that's interesting and might be also something worth addressing. Is there any spec or so reason why we start memory at 0? Is this given by the protocol? Otherwise we could at least safe this first memory copy over step by directly initializing with 96 bytes?

Just ran some mainnet blocks with console.log(Memory extended by ${sizeDiff} to ${newSize}) injected in the extend() method (only when extension is taking place), and going once to 96 is the most common extension operation. 🤔

(I have the impression I am in the range of some attack blocks, I guess therefore the occasional repeated 8768 extensions, no confirmation though on this and at the end a side question)

[jochem-brouwer]

This 96 is due to solidity, it initializes the "free memory pointer" at slot 0x40 (64) which will thus write 32 bytes at offset 64 and thus extend to 96 bytes. I am not sure if I am ok with directly initializing it to 96 bytes, since it is super likely that more memory writes are happening, and if solidity changes their initial memory structure we have this artifact laying around.

[holgerd77]

I will experiment with it a bit.

I tested 1024, that seemed like a reasonable default after some tests and looking into some numbers. The performance tests do not gain by that though, so doesn't seem like such an imminent topic to solve. Would like to do some mainnet block tests as well though.

[holgerd77]

Rebased this via UI.

[holgerd77]

Ok, tests are passing here! 🤩 That was actually an easy fix (in the IDENTITY precompile, thanks to the good description in the associated test).

I also ran 10.000s of tx on mainnet on this, all without problems. So I would say this can be opened for review and eventual merge. 🙂

[jochem-brouwer]

Ah right, I think you encountered the shallow copy Geth bug in the identity? 😄

[jochem-brouwer]

I checked these opcodes and I think this is fine if we do not copy these for essentially the calldata, since this cannot be edited in EVM.

I need to give this a day of more thought though.

Some notes, I also think this non-copy operation can be added to:

• REVERT
• AUTHCALL
• RETURN

For REVERT and RETURN: this writes into the return data buffer, which can only be read, but not edited, so it would make sense to not-copy this there as well.

[holgerd77]

Ah right, I think you encountered the shallow copy Geth bug in the identity? 😄

Yes. 😁

[holgerd77]

For MLOAD I guess we can already do safely because we are converting to BigInt anyhow directly after that:

const pos = runState.stack.pop()
const word = runState.memory.read(Number(pos), 32)
runState.stack.push(bufferToBigInt(word))

Might generally worth to revise if we want to switch over to Buffer/Uint8Array for the stack as well along the breaking releases. But that decision is not an easy take for sure. Might be some base decision for further opimizations and streamlining on the things we are doing here, on the other hand arithmetics get more complicated likely.

[holgerd77]

Quick note: I would really like to get this merged soon since I have various subsequent work I would want to do on top here. I have now run a couple of 100.000 mainnet blocks on this, so I think we are on the safe side. 😋

If we want/decide to do, we can do additional memory optimizations in a subsequent PR.

[g11tech]

shouldn't this alloc go below avoidCopy return?

[holgerd77]

Good point, will update.

[g11tech]

👍

[holgerd77]

Ah, sorry, didn't update the review labels here properly (have done now). @g11tech thanks for the review! 👍