Skip to content
Open
27 changes: 27 additions & 0 deletions src/Nethermind/Nethermind.Evm.Test/CodeAnalysis/CodeInfoTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,33 @@ public void PushNJumpdest_Over10k(int n)
}
}

[Test]
public void GetOrBuildStream_without_code_hash_never_schedules_build()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Low — this still passes with the CodeHash == default early return removed, and it now mutates a process-wide static to do so.

I re-traced it against origin/master's GetOrBuildStream. With BuildThreshold = 1 and no early return:

  1. unhashed.GetOrBuildStream()CodeHash != default is false so the cache lookup is skipped; Interlocked.Increment → 1; 1 < 1 is false; the CAS schedules a StreamBuilder; returns null.
  2. BuildStream() runs, CodeHash != default fails, so it takes the else branch and never calls InstructionStreamCache.Set.
  3. Every later call returns null — either from the StreamBuildUnavailable latch or from the miss.

So SpinUntil(… is not null) is still false and all three assertions hold on the unfixed code. The observable difference the test name promises — that no work item is scheduled — is _streamBuildState staying StreamBuildIdle, and nothing here reads it. The hashed control is a nice addition, but it only proves the harness works, not that the skip happened.

Second, independent problem: StreamInterpreter.BuildThreshold is public static int (VirtualMachine.Stream.cs:26) — process-wide, not per-fixture. Nethermind.Evm.Test has parallelizable fixtures (CodeInfoRepositoryTests, Eip7708Tests with ParallelScope.All, KzgPointEvaluationPrecompileTests, …), and NUnit drains the non-parallel queue on the main thread while parallel workers are still running. So for the duration of this test any concurrently-running EVM test sees a threshold of 1 and starts taking the stream-interpreter path after a single execution — nondeterministic coverage, and a plausible source of "passes locally, flakes in CI". The hashed half also permanently publishes keccak(32× JUMPDEST) into the global InstructionStreamCache.

Note that the mutation buys nothing even for the current assertions: the original version of this test simply called GetOrBuildStream() more than BuildThreshold times, which is threshold-agnostic.

Suggestion: either revert to the loop-past-the-default-threshold form (same discriminating power, no global mutation), or make it actually discriminate — e.g. [InternalsVisibleTo] is already in place for this assembly, so exposing _streamBuildState (or a internal bool StreamBuildScheduled probe) lets you assert Idle after N calls, which fails on master and passes here.

Fix this →

{
int thresholdBefore = StreamInterpreter.BuildThreshold;
StreamInterpreter.BuildThreshold = 1;
try
{
byte[] code = Enumerable.Repeat((byte)Instruction.JUMPDEST, 32).ToArray();
CodeInfo unhashed = new(code);
for (int i = 0; i < 8; i++)
Assert.That(unhashed.GetOrBuildStream(), Is.Null);

Assert.That(
!System.Threading.SpinWait.SpinUntil(() => unhashed.GetOrBuildStream() is not null, TimeSpan.FromMilliseconds(250)),
"default CodeHash must not publish a stream");

CodeInfo hashed = new(code) { CodeHash = Nethermind.Core.Crypto.ValueKeccak.Compute(code) };
Assert.That(
System.Threading.SpinWait.SpinUntil(() => hashed.GetOrBuildStream() is not null, TimeSpan.FromSeconds(5)),
"hashed CodeInfo should build a stream");
}
finally
{
StreamInterpreter.BuildThreshold = thresholdBefore;
}
}
Comment on lines +211 to +236

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Low — this test passes unchanged on master, so it doesn't guard the optimisation it's named after.

On master, GetOrBuildStream() with a default CodeHash also returns null on every call: the cache lookup is skipped by the CodeHash != default guard, the first BuildThreshold - 1 calls return null on the hit counter, and the call that crosses the threshold schedules a background build that BuildStream then refuses to publish (CodeHash != default fails) — returning null too. So every assertion here holds both before and after the change.

To actually pin the behaviour, assert the observable difference: that no build was scheduled. E.g. expose/observe _streamBuildState transitioning to StreamBuildUnavailable on the very first call, or assert InstructionStreamCache gained no entry and no work item ran (the sibling InstructionStreamTests.cs:490 already uses a SpinUntil on GetOrBuildStream and could be inverted here).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed: test now uses BuildThreshold = 1, asserts no stream appears for default CodeHash within a short spin, and a hashed control does build.


[TestCaseSource(nameof(Codes))]
public void JumpDestinationAnalyzer_are_equivalent(byte[] codeInput)
{
Expand Down
63 changes: 62 additions & 1 deletion src/Nethermind/Nethermind.Evm.Test/EvmPooledMemoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,12 @@ public void CalculateMemoryCost_LengthExceedsULong_ShouldReturnOutOfGas()
Assert.That(result, Is.EqualTo(0UL));
}

[TestCase(1024)]
[TestCase(4096)]
[TestCase(32 * 1024)]
[TestCase(70 * 1024)]
[TestCase(2 * 1024 * 1024)]
public void Large_pooled_buffer_is_zeroed_on_reuse(int size)
public void Pooled_buffer_is_zeroed_on_reuse(int size)
{
EvmPooledMemory dirty = new();
UInt256 zero = UInt256.Zero;
Expand All @@ -121,6 +124,64 @@ public void Large_pooled_buffer_is_zeroed_on_reuse(int size)
clean.Dispose();
}

[Test]
public void Grow_after_dirty_reuse_preserves_written_prefix_and_zeroes_tail()
{
const int firstSize = 2048;
const int secondSize = 8192;

EvmPooledMemory dirty = new();
Span<byte> pattern = new byte[firstSize];
pattern.Fill(0xaa);
Assert.That(dirty.TrySave(UInt256.Zero, pattern), Is.True);
dirty.Dispose();

EvmPooledMemory next = new();
try
{
byte[] firstWord = TestItem.KeccakA.BytesToArray();
Assert.That(next.TrySaveWord(UInt256.Zero, firstWord), Is.True);

UInt256 growOffset = (UInt256)(secondSize - EvmPooledMemory.WordSize);
Assert.That(next.TryLoadSpan(in growOffset, (UInt256)EvmPooledMemory.WordSize, out Span<byte> tail), Is.True);
Assert.That(tail.ToArray(), Is.EqualTo(new byte[EvmPooledMemory.WordSize]));

Assert.That(next.TryLoadSpan(UInt256.Zero, (UInt256)EvmPooledMemory.WordSize, out Span<byte> head), Is.True);
Assert.That(head.ToArray(), Is.EqualTo(firstWord));
}
finally
{
next.Dispose();
}
}

[Test]
public void GetTrace_slice_past_size_does_not_leak_dirty_bytes()
{
// Must exceed the 4 KiB RentSlow zero chunk, otherwise the whole buffer is zeroed anyway.
const int dirtySize = 32 * 1024;
EvmPooledMemory dirty = new();
Span<byte> pattern = new byte[dirtySize];
pattern.Fill(0xff);
Assert.That(dirty.TrySave(UInt256.Zero, pattern), Is.True);
dirty.Dispose();

EvmPooledMemory memory = new();
try
{
// TrySaveWord rents (unlike CalculateMemoryCost), so the dirty buffer is reused with Size = 32.
Assert.That(memory.TrySaveWord(UInt256.Zero, new byte[EvmPooledMemory.WordSize]), Is.True);

TraceMemory trace = memory.GetTrace();
Assert.That(trace.Size, Is.EqualTo((ulong)EvmPooledMemory.WordSize));
Assert.That(trace.Slice(0, 8 * 1024).ToArray(), Is.EqualTo(new byte[8 * 1024]), "trace leaked dirty tail bytes past Size");
}
finally
{
memory.Dispose();
}
}

[Test]
public void CalculateMemoryCost_LengthExceedsLongMax_ShouldReturnOutOfGas()
{
Expand Down
4 changes: 3 additions & 1 deletion src/Nethermind/Nethermind.Evm/CodeAnalysis/CodeInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ public CodeInfo(IPrecompile? precompile)
{
if (Volatile.Read(ref _streamBuildState) == StreamBuildUnavailable)
return null;
if (CodeHash != default && InstructionStreamCache.TryGet(CodeHash, out InstructionStream? cached))
if (CodeHash == default)
return null;
if (InstructionStreamCache.TryGet(CodeHash, out InstructionStream? cached))
return cached;
if (Interlocked.Increment(ref _streamHits) < StreamInterpreter.BuildThreshold)
return null;
Expand Down
74 changes: 46 additions & 28 deletions src/Nethermind/Nethermind.Evm/EvmPooledMemory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -393,11 +393,21 @@ private ulong ComputeMemoryExpansionCost(ulong newSize)
return cost;
}

private static readonly TraceMemory EmptyTraceMemory = new(0, default);

public TraceMemory GetTrace()
{
ulong size = Size;
if (size == 0)
return EmptyTraceMemory;

ClearForTracing(size);
return new(size, _memory);
// Clamp to Size so TraceMemory.Slice past the EVM high-water cannot see dirty tail bytes.
if (_memory is null)
return new(size, default);
Comment thread
LukaszRozmej marked this conversation as resolved.

int visible = (int)Math.Min(size, (ulong)_memory.Length);
return new(size, _memory.AsMemory(0, visible));
}

public void Dispose()
Expand All @@ -407,7 +417,7 @@ public void Dispose()
if (memory is not null)
{
_memory = null;
ReturnClean(memory, (int)Math.Min(Size, (ulong)memory.Length));
Return(memory);
}
}

Expand Down Expand Up @@ -453,50 +463,48 @@ private void EnsureRented()

private const int MinRentSize = 1_024;
private const int MaxCachedArrayLength = 1 << 16;
private const int CleanCacheSlots = 16;
private const int CacheSlots = 16;

[ThreadStatic] private static byte[]?[]? _cleanArrays;
[ThreadStatic] private static int _cleanArrayCount;
[ThreadStatic] private static byte[]?[]? _cachedArrays;
[ThreadStatic] private static int _cachedArrayCount;

private static byte[] RentClean(int minLength)
// Cached dirty; RentSlow zero-extends past Size in chunks on growth.
private static byte[] Rent(int minLength)
{
byte[]?[]? cache = _cleanArrays;
int cleanArrayCount = _cleanArrayCount - 1;
for (int i = cleanArrayCount; i >= 0; i--)
byte[]?[]? cache = _cachedArrays;
int cachedArrayCount = _cachedArrayCount - 1;
for (int i = cachedArrayCount; i >= 0; i--)
{
byte[] candidate = cache![i]!;
if (candidate.Length >= minLength)
{
_cleanArrayCount = cleanArrayCount;
cache[i] = cache[cleanArrayCount];
cache[cleanArrayCount] = null;
_cachedArrayCount = cachedArrayCount;
cache[i] = cache[cachedArrayCount];
cache[cachedArrayCount] = null;
return candidate;
}
}

if (minLength > MaxCachedArrayLength)
{
byte[] pooled = RentLarge(minLength);
Array.Clear(pooled);
return pooled;
return RentLarge(minLength);
}

return new byte[BitOperations.RoundUpToPowerOf2((uint)minLength)];
}

private static void ReturnClean(byte[] array, int dirtyLength)
private static void Return(byte[] array)
{
if (array.Length > MaxCachedArrayLength)
{
ReturnLarge(array);
return;
}

byte[]?[] cache = _cleanArrays ??= new byte[CleanCacheSlots][];
if (_cleanArrayCount < CleanCacheSlots)
byte[]?[] cache = _cachedArrays ??= new byte[CacheSlots][];
if (_cachedArrayCount < CacheSlots)
{
Array.Clear(array, 0, dirtyLength);
cache[_cleanArrayCount++] = array;
cache[_cachedArrayCount++] = array;
}
}

Expand Down Expand Up @@ -528,19 +536,29 @@ private static void ReturnLarge(byte[] array)
[MethodImpl(MethodImplOptions.NoInlining)]
private void RentSlow()
{
if (_memory is null)
byte[]? memory = _memory;
if (memory is null)
{
_memory = RentClean((int)Math.Max((uint)Size, MinRentSize));
_memory = memory = Rent((int)Math.Max((uint)Size, MinRentSize));
_lastZeroedSize = 0;
}
else if (Size > (ulong)_memory.LongLength)
else if (Size > (ulong)memory.LongLength)
{
byte[] beforeResize = _memory;
_memory = RentClean(TruncateToInt32(Size));
Array.Copy(beforeResize, 0, _memory, 0, beforeResize.Length);
ReturnClean(beforeResize, beforeResize.Length);
byte[] grown = Rent(TruncateToInt32(Size));
Array.Copy(memory, 0, grown, 0, (int)_lastZeroedSize);
Return(memory);
_memory = memory = grown;
}

_lastZeroedSize = (ulong)_memory.Length;
ulong size = Size;
if (size > _lastZeroedSize)
{
// Over-zero to a chunk boundary so sequential MSTORE growth does not take RentSlow per word.
const ulong zeroChunk = 4 * 1024;
ulong target = Math.Min((ulong)memory.Length, (size + (zeroChunk - 1)) & ~(zeroChunk - 1));
Array.Clear(memory, (int)_lastZeroedSize, (int)(target - _lastZeroedSize));
_lastZeroedSize = target;
}
Comment on lines +553 to +561

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Medium — this moves RentSlow from "once per buffer reallocation" to "once per memory expansion". That is a plausible block-processing regression, and the reported benchmark does not cover it.

EnsureRented fires when memory is null || Size > memory.Length || Size > _lastZeroedSize. Before this PR _lastZeroedSize was always _memory.Length, so the third clause was subsumed by the second and RentSlow was reached only when the buffer actually had to grow. Now the third clause is the live one, so every memory-expanding opcode takes the NoInlining RentSlow call plus an Array.Clear call.

Failure scenario: a contract that ABI-encodes sequentially (MSTORE walking the high-water mark 32 bytes at a time across 32 KiB) goes from ~1000 inlined no-op checks + one 32 KiB Array.Clear at frame exit, to ~1000 non-inlined RentSlow calls + 1000 32-byte Array.Clear calls. "Total bytes cleared are the same" is true, but small Array.Clear is dominated by call/setup cost, not by the stores — so the per-op cost goes up while only one bulk clear is removed.

The PR reports only an eth_call RPC sweep (−2.7% avg). This is the hot path for all block processing, so please run the reproducible payload benchmark (.github/workflows/run-expb-reproducible-benchmarks.yml, or the gas-benchmark skill) before merging, and post the result.

A cheap way to keep the deferred-clear win while amortising the call overhead is to zero-extend to a chunk boundary rather than exactly to Size:

ulong size = Size;
if (size > _lastZeroedSize)
{
    // Over-zero to a chunk boundary so sequential growth doesn't take RentSlow on every word.
    const ulong ZeroChunk = 4 * 1024;
    ulong target = Math.Min((ulong)memory.Length, (size + (ZeroChunk - 1)) & ~(ZeroChunk - 1));
    Array.Clear(memory, (int)_lastZeroedSize, (int)(target - _lastZeroedSize));
    _lastZeroedSize = target;
}

_lastZeroedSize >= Size is preserved, so every other invariant in the file still holds.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f58ff48 (amortization part).

RentSlow now zero-extends to a 4 KiB boundary (capped at memory.Length), so sequential word-by-word MSTORE growth stays inside an already-zeroed chunk and does not take the NoInlining path per op. Invariant _lastZeroedSize >= Size still holds.

On the payload-benchmark ask: agreed this is block-processing hot path; will follow up with expb numbers (or gate on CI) before merge. The chunking change is the cheap insurance in the meantime.

}

// (int)(uint)value rather than (int)value: RyuJIT emits noticeably worse codegen for a
Expand Down
Loading
Loading