Repository navigation
Port upstream EVM memory resize perf fix (PLT-1111) - #100
Conversation
Port upstream geth#33056 to avoid reallocating pooled memory buffers when capacity is already sufficient, and clear backing storage in Free() so reused buffers remain zeroed. Co-authored-by: Cursor <cursoragent@cursor.com>
amir-deris
changed the title
PR SummaryMedium Risk Overview
Adds Reviewed by Cursor Bugbot for commit 50c4239. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Faithful, correct cherry-pick of upstream's Memory.Resize fast path plus the compensating clear() in Free; I traced the "bytes in [len, cap) are always zero" invariant through every mutation path and found it intact, with no memory buffer escaping past mem.Free(). No blockers — but a consensus-critical invariant is now load-bearing, implicit, and untested, which is worth addressing before merge.
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
cursor-review.mdis empty — the Cursor second-opinion pass produced no output, so this synthesis rests on the Codex pass ("no material issues found") and my own analysis.REVIEW_GUIDELINES.mdis also empty, so no repo-specific standards were applied.- I was unable to run
go test ./core/vm/in this environment (command required approval that was not granted). Findings are from static analysis only; please confirm the package suite andBenchmarkResizepass in CI. - Minor behavioral change worth knowing:
Freenow actively zeroes the buffer, where previously it only resliced to[:0]and left bytes intact until overwritten. I confirmed this is safe here — themem.Free()defer atcore/vm/interpreter.go:212is registered before the tracer defers, so it runs afterOnOpcode/OnFault, andScopeContext.MemoryData()is documented as read-only. But any future tracer that retains the slice past its callback would now observe zeros rather than stale data. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| m.store = append(m.store, make([]byte, size-uint64(m.Len()))...) | ||
| if uint64(len(m.store)) < size { | ||
| if uint64(cap(m.store)) >= size { | ||
| m.store = m.store[:size] |
There was a problem hiding this comment.
[suggestion] This fast path is correct, but it makes an implicit invariant load-bearing for consensus: bytes in [len(m.store), cap(m.store)) must always be zero. I traced it and it currently holds — Free clears [0, len), the append branch below only runs when cap < size (so growslice always reallocates, and Go zeroes [newlen, cap) for pointer-free element types), and Set/Set32/MSTORE8 never write past len.
The risk is that nothing in the code says so. A future change that shrinks len without clearing — e.g. a Reset() doing m.store = m.store[:0], or a truncating resize — would silently expose stale bytes to the EVM and diverge nodes, with no compiler or test to catch it. Worth a short comment here (and at the clear() in Free) stating the invariant and why clearing only len bytes is sufficient.
| } | ||
| } | ||
|
|
||
| func BenchmarkResize(b *testing.B) { |
There was a problem hiding this comment.
[suggestion] The PR body correctly identifies the clear() in Free as required for correctness ("EVM execution results can diverge across nodes"), but the only test added is a benchmark — nothing asserts the zeroing invariant itself.
Consider adding a test that exercises the pool reuse cycle directly: Resize a Memory to some size, write non-zero bytes into it, Free() it, then NewMemory() + Resize back into the same range and assert every byte is zero. That pins the exact regression this change guards against, and would fail loudly if the clear() were ever dropped or the reslice path extended to shrink.
|
|
||
| func BenchmarkResize(b *testing.B) { | ||
| memory := NewMemory() | ||
| for i := range b.N { |
There was a problem hiding this comment.
[nit] This benchmark never frees or resets, so memory grows monotonically to b.N bytes. At default -benchtime=1s with ~ns-per-op, b.N can reach the hundreds of millions, and the doubling growth means roughly 2x that in peak RSS — enough to matter on a constrained CI runner, and worse under -benchtime=10s.
Upstream's version has the same shape, so this is fine to keep as-is for port fidelity. If you'd rather bound it, cycling the size (e.g. memory.Resize(uint64(i % 4096)) after an initial warm Resize) still exercises the cap >= size fast path this change adds, which is the interesting one.
|
@seidroid review |
|
Claude finished @amir-deris's task in 3m 43s —— View job Review — PLT-1111 EVM memory resize port
Verdict: looks good to merge. 0 blocking, 5 non-blocking. This re-review covers What I verifiedThe correctness of the fast path rests on one invariant: bytes in
I also checked that nothing aliases the buffer past
So the new The clamping in Non-blocking1. The load-bearing invariant is still undocumented in Clamping closes the 2. The pool-reuse zeroing path — the thing
func TestMemoryFreeClears(t *testing.T) {
m := NewMemory()
m.Resize(64)
for i := range m.store {
m.store[i] = 0xff
}
m.Free()
// Free must zero the bytes before they are handed back by a resize-within-cap.
if tail := m.store[:64]; !bytes.Equal(tail, make([]byte, 64)) {
t.Errorf("Free did not clear backing array: %#x", tail)
}
}That fails loudly if the 3. The clamping is a sei-local divergence from upstream. Upstream ethereum#33056 changed only 4.
5. Two small things in the new test ( Notes
|
There was a problem hiding this comment.
The Resize fast path and clear()-on-Free port looks correct: Resize is only ever called with monotonically growing sizes from one call site, and every consumer that outlives a frame copies (opReturn/opRevert/opCreate/makeLog use GetCopy, AddPreimage clones, and all tracers consume MemoryData() synchronously), so the new clear() in Free cannot zero data still in use. Remaining notes are test/lint nits and one divergence from upstream worth calling out.
Findings: 0 blocking | 8 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), and the Codex pass reported no material findings but could not run the memory tests (sandboxed Go module cache).REVIEW_GUIDELINES.mdis also empty, so no repo-specific standards were applied. - I could not execute
go test ./core/vm/or golangci-lint in this environment either, so the new test and benchmark were only reviewed statically — please confirm CI is green before merging. - Missing test for the change that actually matters for consensus: nothing asserts that
Free()zeroes the buffer before returning it to the pool.TestMemoryViewCapacityClampedonly covers the capacity clamp. Consider a small test that dirties memory, callsFree(), and asserts the full backing array (store[:cap(store)]) is zero — that is the guarantee theResizefast path depends on. - Worth documenting the new load-bearing invariant on the
storefield itself (bytes in [len, cap) are always zero), sinceSet,Set32andCopystill operate on unclampedm.storeslices and future in-package edits are the remaining way to break it. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| m.store = append(m.store, make([]byte, size-uint64(m.Len()))...) | ||
| if uint64(len(m.store)) < size { | ||
| if uint64(cap(m.store)) >= size { | ||
| m.store = m.store[:size] |
There was a problem hiding this comment.
[suggestion] This fast path is only sound because m.store[len(m.store):cap(m.store)] is guaranteed to be all zeroes, which in turn depends on the new clear() in Free and on the capacity clamping below. That's the consensus-critical part of this change and it's currently only explained in the PR description. Worth a one-line comment here (and on clear(m.store) in Free) so a future edit doesn't silently break it.
| t.Errorf("%s: capacity not clamped: have %d, want %d", tc.name, have, want) | ||
| } | ||
| // Appending must reallocate rather than write into m.store's tail. | ||
| _ = append(tc.view, 0xff) |
There was a problem hiding this comment.
[suggestion] _ = append(...) with a discarded result is exactly what staticcheck's SA4010 flags ("this result of append is never used"), and .golangci.yml enables staticcheck with run.tests: true — this may fail lint in CI. It's also redundant: the cap(tc.view) == len(tc.view) assertion above already proves an append must reallocate. Suggest dropping the line, or making it load-bearing, e.g. if v := append(tc.view, 0xff); &v[0] == &tc.view[0] { t.Errorf(...) }.
| func BenchmarkResize(b *testing.B) { | ||
| memory := NewMemory() | ||
| for i := range b.N { | ||
| memory.Resize(uint64(i)) |
There was a problem hiding this comment.
[suggestion] Resize(uint64(i)) grows monotonically with b.N, so the backing array ends up b.N bytes — at ~1ns/op and the default -benchtime=1s that's on the order of a gigabyte, and memory.Free() is never called so it's held for the whole run. Consider bounding the size (e.g. memory.Resize(uint64(i%1024)), with a Free()/fresh NewMemory() when it wraps) so a go test -bench=. run can't blow up CI memory; that also exercises the reslice fast path more representatively.
| return m.store | ||
| // Clamp the capacity so callers cannot append into the unallocated tail of | ||
| // the backing array, which Resize may later hand back as zeroed memory. | ||
| return m.store[:len(m.store):len(m.store)] |
There was a problem hiding this comment.
[nit] The three-index clamping in Store, Data and GetPtr is defensive hardening that upstream ethereum#33056 does not include. It's cheap and correct, and I agree with the reasoning (no current caller appends — I checked GetMemoryCopyPadded and all tracer consumers), but it does add fork-local divergence in a file that will keep receiving cherry-picks. Consider noting in the commit message that these three lines are Sei-local additions, or upstreaming them, so future merges don't silently drop them.
2 participants
Summary
Cherry-picks upstream ethereum/go-ethereum#33056 (
3bbf5f5b6) to speed up EVM memory expansion on the block-execution hot path.Memory.Resizepreviously always appended a fresh zeroed slice, even when a pooled backing array already had sufficient capacity — allocating and copying unnecessarily. The fix reslices within existing capacity whencap(m.store) >= size, and only falls back toappendwhen a larger backing array is required.Memory.Freenow callsclear(m.store)before resetting length and returning the instance to the pool. This is required for correctness: reslicing exposes previously used bytes that must be zeroed before reuse, or EVM execution results can diverge across nodes.Upstream measured this as the single largest EVM perf win in their gap: ~70% faster
Resizemicrobenchmark, and ~10% of total block execution time in blocktest profiling.Changes
core/vm/memory.go: fast-path reslice inResize;clear()inFreecore/vm/memory_test.go: addBenchmarkResize(upstream benchmark)