Skip to content

Port upstream EVM memory resize perf fix (PLT-1111) #100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
amir-deris merged 2 commits into from
Sep 21, 2026
Merged

Port upstream EVM memory resize perf fix (PLT-1111) #100

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a287d4
core/vm: reslice EVM memory within cap on resize (PLT-1111)
amir-deris Sep 15, 2026
50c4239
fix the issue with append to unallocated tail of the backing array
amir-deris Sep 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions core/vm/memory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ func (m *Memory) Free() {
// To reduce peak allocation, return only smaller memory instances to the pool.
const maxBufferSize = 16 << 10
if cap(m.store) <= maxBufferSize {
clear(m.store)
m.store = m.store[:0]
m.lastGasCost = 0
memoryPool.Put(m)
Expand Down Expand Up @@ -80,10 +81,14 @@ func (m *Memory) Set32(offset uint64, val *uint256.Int) {
val.PutUint256(m.store[offset:])
}

// Resize resizes the memory to size
// Resize grows the memory to the requested size.
func (m *Memory) Resize(size uint64) {
if uint64(m.Len()) < size {
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]

@seidroid seidroid Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@seidroid seidroid Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

} else {
m.store = append(m.store, make([]byte, size-uint64(len(m.store)))...)
}
}
}

Expand Down
7 changes: 7 additions & 0 deletions core/vm/memory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,10 @@ func TestMemoryCopy(t *testing.T) {
}
}
}

func BenchmarkResize(b *testing.B) {

@seidroid seidroid Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

memory := NewMemory()
for i := range b.N {

@seidroid seidroid Bot Sep 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

memory.Resize(uint64(i))

@seidroid seidroid Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

}
}
Loading