perf(rpc): cut eth_call overhead in memory, code stream, and tx parse - #12716
perf(rpc): cut eth_call overhead in memory, code stream, and tx parse#12716kamilchodola wants to merge 9 commits into
Conversation
Zero EVM pooled memory on growth instead of frame exit, skip uncacheable instruction-stream builds, derive TransactionForRpc subtype from the Utf8JsonReader instead of a DOM, and keep repeat flat-state account promotions off the bucket lock. Regression coverage for dirty-buffer reuse, default-hash CodeInfo, and large-calldata tx type detection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Claude finished @kamilchodola's task in 16m 41s —— View job Review:
|
| # | Finding |
|---|---|
| 4 | using System.Text.Json.Nodes; is now unused → code-lint.yml will fail CI |
| 5 | reader.Skip() throws on non-final-block readers; TrySkip() is free. Current call sites are all isFinalBlock: true, so latent |
| 6 | Missed free win: break once setType is found — avoids scanning the 100s-of-KB input this PR is optimizing for |
| 7 | Escaped-name fallback allocates 2 strings per (property × discriminator); also worth documenting that the | 0x20 fold relies on ASCII-letter discriminators |
| 8 | DiscriminatorProperties is now dead code (AGENTS.md: remove it); Math.Min(_txTypes.Count, 64) is a silent cap |
| 9 | GetOrBuildStream_without_code_hash_is_unavailable passes on master — doesn't discriminate the change |
What I verified as correct
Worth recording, since these were the parts most likely to be wrong:
EvmPooledMemoryEVM-visible invariant holds. Every write/read path (TrySave*,TryLoad*,*AfterGas,CopyAfterGas,Load32BytesAfterGas) is preceded byEnsureRented, which zero-extends to the fullSize— not just to the accessednewLength. I specifically checked MCOPY, the one op where the source region can sit above the destination:UpdateMemoryCost(..., UInt256.Max(b, a), c, ...)raisesSizeto covermax(src,dst)+lenbeforeCopyAfterGas, so the source read is inside the zeroed prefix.Inspectis guarded bylargeSize > _memory.Lengthand callsClearForTracing._lastZeroedSize = 0in thememory is nullbranch is load-bearing and present —VmState.Disposedoes_memory = default(VmState.cs:229), so a pooledVmStatere-enters withSize = 0; and the grow branch'sArray.Copy(..., (int)_lastZeroedSize)is in bounds in every state I could construct, including theClearForTracing-clamped andCalculateMemoryCost(rentIfNeeded: false)cases.- Large arrays returned dirty to
ArrayPool.Sharedis not new — the oldReturnCleanalready ignoreddirtyLengthfor> MaxCachedArrayLength; only the rent-side fullArray.Clearwas dropped, andRentSlowcovers[0, Size). - Tx-type derivation is order-equivalent.
_txTypesis built in descendingTxTypeorder (SetCode, Blob, EIP1559, AccessList, Legacy), soTrailingZeroCount(discriminated)picks the same winner as the oldFirstOrDefault.type→gasPrice→ discriminator precedence is preserved, and case-insensitivity matches the oldJsonObjectlookup (EthereumJsonSerializersetsPropertyNameCaseInsensitive = true).GasPricecarries no[JsonDiscriminator], so skipping the discriminator loop in thegasPricebranch is equivalent. PromoteAccountis semantically identical and genuinely cheaper.ContainsKeyis lock-free,TryAddstill guards the race, and hoistingHashedKey<Address>reuses the cached_hashCode(HashedKey.cs:11) across both lookups.
Merge recommendation
Blocked on #1 (correctness) and #4 (CI lint). #2 needs a block-processing benchmark or an explicit "measured, no regression" note; #3 is a one-line hardening. The Lows are optional but #6 is free performance in exactly the scenario this PR targets.
· branch perf/eth-call-memory-and-rpc-parse
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Cached dirty; RentSlow zero-extends [_lastZeroedSize, Size) 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) |
There was a problem hiding this comment.
High — the "no caller can observe bytes beyond the zeroed prefix" invariant does not hold for TraceMemory.
GetTrace() (line 396) does ClearForTracing(Size) and then hands the whole array to TraceMemory:
public TraceMemory GetTrace()
{
ulong size = Size;
ClearForTracing(size);
return new(size, _memory); // ReadOnlyMemory over _memory.Length, not size
}TraceMemory.Slice(start, length) (Nethermind.Evm/Tracing/TraceMemory.cs:57) bounds-checks against span.Length — the array length — not Size:
ReadOnlySpan<byte> span = _memory.Span;
if (start + length > span.Length) { /* pad with zeros */ }
return span.Slice(start, length); // <-- raw tailBefore this PR every rented buffer was fully zeroed (RentClean cleared the whole array, and RentSlow set _lastZeroedSize = _memory.Length), so an out-of-Size slice read back as zeros. Now _lastZeroedSize == Size and everything past it is the previous frame's / previous transaction's memory.
Failure scenario: debug_traceTransaction / debug_traceCall with a custom JS tracer whose step does log.memory.slice(0, 512) while log.memory.length() is 32 (Custom/JavaScript/Log.cs:69 → MemoryTrace.Slice). Old output: 32 real bytes + 480 zero bytes. New output: 32 real bytes + 480 bytes of whatever the thread-local cached buffer (MinRentSize = 1024) held from a prior frame — so trace output becomes non-deterministic and leaks unrelated EVM memory into the RPC response. NativePrestateTracer:160 and GethLikeTxDirectStreamingTracer:195 are safe because they clamp to Size.
Cheapest fix is to clamp what TraceMemory can see, which also restores the old zero-padding path:
public TraceMemory GetTrace()
{
ulong size = Size;
ClearForTracing(size);
return new(size, _memory is null ? default : _memory.AsMemory(0, (int)Math.Min(size, (ulong)_memory.Length)));
}Please also add a regression test (dirty a large buffer, dispose, then GetTrace().Slice(0, n) past Size and assert zeros).
Fix this →%20is%20zeroed.%20TraceMemory.Slice%20bounds-checks%20against%20the%20array%20length%2C%20so%20a%20JS%20tracer%20calling%20log.memory.slice%20past%20Size%20now%20reads%20stale%20bytes%20instead%20of%20zeros.%20Clamp%20the%20ReadOnlyMemory%20passed%20to%20TraceMemory%20to%20min(Size%2C%20_memory.Length)%20and%20add%20a%20regression%20test%20in%20EvmPooledMemoryTests.&repo=NethermindEth/nethermind)
There was a problem hiding this comment.
Fixed in f58ff48.
GetTrace() now clamps the ReadOnlyMemory passed to TraceMemory to min(Size, _memory.Length), so Slice past the EVM high-water takes the zero-pad path instead of reading dirty tail bytes.
Regression: GetTrace_slice_past_size_does_not_leak_dirty_bytes — dirties a 1 KiB buffer, reuses it with Size=32, asserts trace.Slice(0, 512) is all zeros.
| ulong size = Size; | ||
| if (size > _lastZeroedSize) | ||
| { | ||
| Array.Clear(memory, (int)_lastZeroedSize, (int)(size - _lastZeroedSize)); | ||
| _lastZeroedSize = size; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // No cache key (e.g. eth_call overrides) — building would only be discarded. | ||
| if (CodeHash == default) | ||
| { | ||
| Volatile.Write(ref _streamBuildState, StreamBuildUnavailable); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Medium — latching StreamBuildUnavailable here permanently poisons the instance, and CodeHash is a public setter assigned after construction.
CodeHash is public ValueHash256 CodeHash { get; set; } and is filled in by StaticCodeCache.Set (StaticCodeCache.cs:21), i.e. after new CodeInfo(code). Today every cacheable path happens to Set before the instance is ever executed (CacheCodeInfoRepository.GetOrCacheCodeInfo, InsertCode, SetDelegation), so this is latent rather than live. But the moment any path executes a CodeInfo once before publishing it to the code cache — or publishes it to a cache implementation that doesn't assign CodeHash — that bytecode loses the stream interpreter for the entire lifetime of the cached instance, silently and with no way to recover. That's a hard-to-diagnose performance cliff on a hot contract.
The Volatile.Write buys essentially nothing: it saves one ValueHash256 comparison on a path that has already taken a Volatile.Read and is about to take a cache lookup. Making the skip stateless removes the failure mode entirely:
// No cache key (e.g. eth_call overrides) — building would only be discarded.
if (CodeHash == default)
return null;If you do want to keep the latch, CodeHash should become set-once/immutable so the ordering is enforced rather than assumed.
There was a problem hiding this comment.
Fixed: skip is now stateless (if (CodeHash == default) return null;) — no latch of StreamBuildUnavailable, so a late CodeHash assignment can still enable stream builds.
| using System.Reflection; | ||
| using System.Text; | ||
| using System.Text.Json; | ||
| using System.Text.Json.Nodes; |
There was a problem hiding this comment.
Low — using System.Text.Json.Nodes; is now unused and will fail the lint job.
JsonObject / JsonNode no longer appear anywhere in this file after DeriveTxType was rewritten. .github/workflows/code-lint.yml flags unused using directives, so CI will go red on this. Please drop the line.
| using System.Text.Json.Nodes; |
| } | ||
|
|
||
| reader.Read(); | ||
| reader.Skip(); |
There was a problem hiding this comment.
Low — Utf8JsonReader.Skip() throws InvalidOperationException on a non-final-block reader; TrySkip() doesn't.
Per the BCL contract, Skip() throws "The reader was given partial data with more data to follow (that is, IsFinalBlock is false)". The previous JsonSerializer.Deserialize<JsonObject>(ref txTypeReader, options) had no such constraint.
Today all reachable call sites are safe — the JSON-RPC pipeline builds its reader with isFinalBlock: true (JsonRpcProcessor.cs:449) and batch items go through JsonDocument.Parse — and I couldn't find any IJsonSerializer.Deserialize<T>(Stream) call whose T transitively contains a TransactionForRpc. So this is a latent trap rather than a live bug, but the converter is a public extension point and the failure would surface as a 500 on eth_call, not a JSON parse error.
TrySkip() costs nothing here and removes the constraint:
| reader.Skip(); | |
| if (!reader.TrySkip()) break; |
There was a problem hiding this comment.
Fixed: reader.TrySkip() instead of Skip().
| { | ||
| reader.Read(); | ||
| setType = JsonSerializer.Deserialize<TxType?>(ref reader, options); | ||
| continue; |
There was a problem hiding this comment.
Low (perf, free win) — once setType is non-null the answer is fully determined, so the scan can stop here.
setType is not null short-circuits ahead of both hasGasPrice and viaDiscriminator, so nothing read after this point can change the result. Continuing means the reader still has to scan to the closing quote of input — which on the simulation-style payloads this PR is optimising for is hundreds of kilobytes, and type is conventionally the first property.
| continue; | |
| if (setType is not null) break; | |
| continue; |
There was a problem hiding this comment.
Fixed: break the property scan once setType is non-null.
EXPB Benchmark ComparisonRun: View workflow run superblocksNo metrics were produced for realblocksNo metrics were produced for fusakaNo metrics were produced for |
| { | ||
| if (reader.HasValueSequence || reader.ValueIsEscaped) | ||
| { | ||
| return string.Equals(reader.GetString(), Encoding.UTF8.GetString(lowerCaseName), StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Low — the escaped-name fallback allocates two strings per comparison, and it is called once per (property × discriminator) pair.
Encoding.UTF8.GetString(lowerCaseName) re-materialises a compile-time-constant literal on every call, and reader.GetString() allocates the unescaped name again for each of the ~7 discriminators plus type/gasprice. An RPC caller can trigger this path deliberately by escaping every property name ("type", "input", …), turning one request into O(properties × discriminators) string allocations.
reader.ValueTextEquals(utf8) handles both escaping and HasValueSequence allocation-free, but is ordinal — so the cleanest shape is to try the ordinal UTF-8 compare first and only fall back for genuinely mixed-case escaped names, or to unescape once per property (into a stack buffer) before entering the discriminator loop rather than once per comparison.
Also worth noting for future maintainers: the | 0x20 fold is only sound because every current discriminator name is pure ASCII letters. Since RegisterTransactionType<T> is an extension point (Optimism registers its own tx type), the invariant is worth stating in the doc comment — b | 0x20 == target has a second pre-image for non-letter targets.
There was a problem hiding this comment.
Fixed: hot path stays allocation-free; escaped path tries ValueTextEquals first, then one GetString with ASCII fold (no per-call Encoding.UTF8.GetString of the literal). Noted ASCII-letters invariant on the fold.
| public Type Type { get; set; } | ||
| public FromTransactionFunc FromTransactionFunc { get; set; } | ||
| public string[] DiscriminatorProperties { get; set; } = []; | ||
| public byte[][] DiscriminatorPropertiesUtf8 { get; set; } = []; |
There was a problem hiding this comment.
Low — DiscriminatorProperties is now dead; per AGENTS.md ("If a change makes existing code unused, remove it") it should go.
After the rewrite, DiscriminatorProperties is only ever assigned — nothing reads it (I checked the whole repo; the only other hits are the assignment at line 163 and the <see cref> in the doc comment that the follow-up commit removed). Drop the string[] property and build the UTF-8 array straight from uniqueProperties in RegisterTransactionType.
Separately: Math.Min(_txTypes.Count, 64) at line 230 silently stops matching discriminators for any type registered at index ≥ 64. With 5 built-in types plus plugin registrations that's not reachable today, but the truncation is invisible. A Debug.Assert(_txTypes.Count <= 64) in RegisterTransactionType would make the bitmask's capacity an enforced invariant rather than a silent cap.
There was a problem hiding this comment.
Fixed: removed dead DiscriminatorProperties; build UTF-8 discriminators directly. Added Debug.Assert(_txTypes.Count < 64) on registration.
| [Test] | ||
| public void GetOrBuildStream_without_code_hash_is_unavailable() | ||
| { | ||
| byte[] code = Enumerable.Repeat((byte)Instruction.JUMPDEST, StreamInterpreter.BuildThreshold + 8).ToArray(); | ||
| CodeInfo codeInfo = new(code); | ||
|
|
||
| for (int i = 0; i < StreamInterpreter.BuildThreshold + 2; i++) | ||
| { | ||
| Assert.That(codeInfo.GetOrBuildStream(), Is.Null); | ||
| } | ||
|
|
||
| Assert.That(codeInfo.GetOrBuildStream(), Is.Null); | ||
| } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Fixed: test now uses BuildThreshold = 1, asserts no stream appears for default CodeHash within a short spin, and a hashed control does build.
TraceMemory.Slice bounds-checks against the backing array, so hand it only [0, Size). Amortize deferred clears by zeroing to a 4 KiB boundary so sequential MSTORE growth does not take RentSlow per word. Regression for JS-tracer style slice-past-Size on a dirty reused buffer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Claude finished @kamilchodola's task in 4m 52s —— View job Re-review:
|
| # | Sev | Finding |
|---|---|---|
| 3 | Medium | CodeInfo permanently latches StreamBuildUnavailable on a default CodeHash — CodeHash is a public setter assigned after construction by StaticCodeCache.Set. Latent today; a one-line return null removes the failure mode |
| 4 | Low (CI) | using System.Text.Json.Nodes; is unused — still on line 11 of TransactionForRpc.cs. Confirmed this reds CI: .editorconfig:97 sets IDE0005.severity = warning and code-lint.yml fails on grep -qP "warning (IDE|CA)\d+". The Code Lint run hasn't reported yet on this head |
| 5 | Low | reader.Skip() throws on non-final-block readers — TrySkip() is free |
| 6 | Low | Free win: break once setType is found — avoids scanning the 100s-of-KB input this PR targets |
| 7 | Low | Escaped-name fallback allocates 2 strings per (property × discriminator) |
| 8 | Low | DiscriminatorProperties is dead code (AGENTS.md: remove it); Math.Min(_txTypes.Count, 64) is a silent cap |
| 9 | Low | GetOrBuildStream_without_code_hash_is_unavailable passes on master |
Informational
The clamp narrows TraceMemory._memory.Length from the array length to Size, which shifts Slice's MemoryPadLimit (1 MiB) reference point. A tracer slicing more than 1 MiB past Size now throws InvalidOperationException where it previously would have padded, but only in the window _memory.Length - Size, which is bounded by max(MinRentSize, ~Size). Not reachable in any realistic tracer, and arguably the more correct semantics (memory past the high-water is conceptually zero, and the pad limit exists to bound the allocation). No action needed — noting it so it isn't a surprise later.
Merge recommendation
Not blocked on correctness any more. Before merge:
- committed on a wrong branch #10 — amend the test so Feature/jsonrpc #1 actually has a guard (the fix itself is right; the test just doesn't prove it)
- Hive #4 — one-line delete, or Code Lint goes red
- Netcore #3 — one-line hardening
#5–#9 are optional; #6 is free performance in exactly the scenario this PR targets.
· branch perf/eth-call-memory-and-rpc-parse
| const int dirtySize = 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 | ||
| { | ||
| memory.CalculateMemoryCost(0, 32, out bool outOfGas); | ||
| Assert.That(outOfGas, Is.False); | ||
|
|
||
| TraceMemory trace = memory.GetTrace(); | ||
| Assert.That(trace.Size, Is.EqualTo(32UL)); | ||
| Assert.That(trace.Slice(0, 512).ToArray(), Is.EqualTo(new byte[512])); |
There was a problem hiding this comment.
Medium — this regression test passes on the unfixed code, so it does not guard the GetTrace clamp. Two independent reasons, either one alone is enough to make it vacuous:
1. The dirty buffer is never rented. CalculateMemoryCost(0, 32, …) → ComputeMemoryExpansionCost → UpdateSize(newSize, rentIfNeeded: false) (EvmPooledMemory.cs:391). So Size becomes 32 but _memory is still null when GetTrace() runs. The assertion is satisfied by the brand-new _memory is null early return — and it was equally satisfied before the fix, because new(size, _memory) with a null array produced an empty ReadOnlyMemory<byte> and Slice took the zero-pad path anyway.
2. Even with a real rent, 1 KiB can't hold a dirty byte. dirtySize == MinRentSize == 1024, and the new 4 KiB chunk rounding in RentSlow zeroes min(memory.Length, roundUp(32, 4096)) = min(1024, 4096) = 1024 — the entire buffer. So no offset in that array is dirty, clamp or not.
To make it discriminate, force an actual rent and use a buffer bigger than the 4 KiB zero chunk:
| const int dirtySize = 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 | |
| { | |
| memory.CalculateMemoryCost(0, 32, out bool outOfGas); | |
| Assert.That(outOfGas, Is.False); | |
| TraceMemory trace = memory.GetTrace(); | |
| Assert.That(trace.Size, Is.EqualTo(32UL)); | |
| Assert.That(trace.Slice(0, 512).ToArray(), Is.EqualTo(new byte[512])); | |
| // 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"); | |
| } |
Rent scans the thread-local cache from the top down and takes the first entry with Length >= minLength, and the dirtied 32 KiB array is the most recently returned — so it is deterministically the one picked, same assumption the sibling Pooled_buffer_is_zeroed_on_reuse already relies on. I verified this by reading; I couldn't run dotnet test in this job (command not in the allowlist), so please confirm the amended test fails with the GetTrace clamp reverted before merging.
There was a problem hiding this comment.
Fixed: test now dirties 32 KiB (past the 4 KiB zero chunk), forces a real rent via TrySaveWord, and asserts Slice(0, 8 KiB) is zeros. Confirmed locally with EvmPooledMemoryTests green.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@claude re-review |
|
Claude finished @kamilchodola's task in 5m 39s —— View job Re-review: head
|
| # | Sev | Status |
|---|---|---|
| 1 | ✅ fixed in f58ff48 (GetTrace clamp) |
|
| 2 | ✅ fixed in f58ff48 + settled by the expb run |
|
| 4 | ✅ fixed in 54e9009 |
|
| 10 | Medium | ❌ open — the regression test for #1 passes on the unfixed code |
| 3 | Low (was Medium) | ❌ open — downgraded, see below |
| 5, 6, 7, 8, 9 | Low | ❌ open |
Medium — the only item I'd block on
#10 — EvmPooledMemoryTests.cs:161 GetTrace_slice_past_size_does_not_leak_dirty_bytes is vacuous
I re-traced it against the current file and both reasons still hold exactly:
CalculateMemoryCost(0, 32, …)→ComputeMemoryExpansionCost→UpdateSize(newSize, rentIfNeeded: false)(EvmPooledMemory.cs:391).Sizebecomes 32,_memorystaysnull. The assertion is satisfied by the new_memory is null || size == 0early return at line 401 — and was equally satisfied before the clamp, becausenew(size, (byte[])null)is an emptyReadOnlyMemory<byte>andTraceMemory.Slicezero-pads.dirtySize == 1024;RentSlowzeroesmin(memory.Length, roundUp(32, 4096)) = min(1024, 4096) = 1024— the whole buffer. Nothing dirty is reachable at any offset.
I hand-simulated the fix I suggested inline (32 KiB dirty + TrySaveWord to force a real rent + Slice(0, 8192)) against the current code and it does discriminate: _lastZeroedSize lands at 4096, [4096, 32768) stays 0xff, and without the clamp Slice(0, 8192) returns those bytes. AGENTS.md ("When fixing a bug, always add a regression test") is the reason this matters — otherwise a future refactor of GetTrace silently reopens the leak. dotnet is not in this job's allowlist, so please confirm the amended test fails with the clamp reverted.
Low — #3 downgraded from Medium
CodeInfo.cs:82 latches StreamBuildUnavailable on a default CodeHash — I went looking for a live victim this time and there isn't one:
- Only two
ICodeCacheimpls exist (StaticCodeCache,NoopCodeCache).StaticCodeCache.SetassignsCodeHash(StaticCodeCache.cs:21) before publishing, and all threeSetcall sites (CacheCodeInfoRepositorylines 37/60/68) publish before the instance is reachable for execution. - The
CodeInfos that do execute with a defaultCodeHash—StateOverridesExtensions.cs:117(SetCodeOverride),OverridableCodeInfoRepository.cs:51(MovePrecompile),VirtualMachine.warmup.cs:47— are all per-request instances, so a per-instance latch is discarded with them. That's exactly the case this PR is optimising. - The
NoopCodeCache(stateless/witness) path never built a stream onmastereither:BuildStreamrefuses to publish whenCodeHash == default, so behaviour is unchanged.
So this is latent hardening, not a defect: the risk is only that CodeHash's public setter makes "published before first execution" an assumed rather than enforced ordering. The Volatile.Write buys one ValueHash256 compare on an already-slow path; return null without the write removes the failure mode. Your call.
Low — unchanged, all still open
| # | Finding |
|---|---|
| 5 | reader.Skip() at line 244 throws on non-final-block readers; TrySkip() is free. All current call sites are isFinalBlock: true, so latent |
| 6 | break once setType is found (line 220 still continue) — free win, and it avoids scanning the 64 KiB input your own new test case at TransactionForRpcDeserializationTests.cs:82 exercises |
| 7 | Escaped-name fallback re-materialises both strings per (property × discriminator). I re-checked the | 0x20 fold against every registered discriminator (AccessList, MaxFeePerGas, MaxPriorityFeePerGas, BlobVersionedHashes, AuthorizationList) — all pure ASCII letters, so sound today. It silently breaks for any name containing _ (0x5F | 0x20 == 0x7F), and RegisterTransactionType<T> is a plugin extension point, so the constraint is worth stating in the doc comment |
| 8 | DiscriminatorProperties is dead now that DiscriminatorPropertiesUtf8 exists — still assigned at line 162, read nowhere. AGENTS.md: remove it. Also Math.Min(_txTypes.Count, 64) (line 229) is a silent cap; a Debug.Assert in RegisterTransactionType would make the bitmask capacity an enforced invariant |
| 9 | GetOrBuildStream_without_code_hash_is_unavailable passes on master — re-traced: on master the CodeHash != default guard skips the cache lookup, the first BuildThreshold - 1 calls return null on the hit counter, and the threshold-crossing call schedules a build that BuildStream then refuses to publish. Every assertion holds both before and after |
Re-verified from source this pass
Not taking the previous pass on trust, since the merge commit could have shifted things:
_lastZeroedSizecan now exceedSize(chunk rounding) — a state that was previously impossible. Every consumer survives it:EnsureRented(Size > _lastZeroedSize) just no-ops more often;ClearForTracing(line 272) is guarded bysize > _lastZeroedSizeand caps atmemory.Length; the grow branch'sArray.Copy(memory, 0, grown, 0, (int)_lastZeroedSize)is in bounds because the grow path requiresSize > memory.Lengthand_lastZeroedSize <= memory.Lengthalways.- The grow branch still copies all live data. Copying only
_lastZeroedSizebytes is safe because every write path goes throughEnsureRented, which zero-extends to the fullSize— so_lastZeroedSizeis always ≥ the written high-water mark. - MCOPY — the one op whose source can sit above the destination, and
CopyAfterGasonly callsPrepareAccessAfterGas(dest + len). Re-checkedEvmInstructions.Storage.cs:303:UpdateMemoryCost(ref gas, UInt256.Max(b, a), c, …)raisesSizeto covermax(src,dst)+len, andEnsureRentedzeroes toSize(not to the accessed length), so the source read is inside the zeroed prefix. Same for theTTracingInst.IsActivebranch'sLoadSpanAfterGas(in b, length). - No new span-invalidation window. The new
Size > _lastZeroedSizeclause reachesRentSlowbut never reallocates (reallocation still requiresSize > memory.Length), so spans held across a subsequentEnsureRented— e.g.sourcein the MCOPY tracing branch — stay valid. Returnno longer clears, and large buffers now come back dirty from the sharedArrayPool. EVM-visible reads are still covered byRentSlow's[0, Size)zeroing, and the trace path is clamped, so no cross-transaction/cross-thread bytes are observable.Inspectremains guarded (largeSize > _memory.Length→default, thenClearForTracing).Size == 0rent leaves a fully dirty 1 KiB buffer with_lastZeroedSize == 0; every read at that point is a zero-length span, and the next expansion clears from 0. Safe.PromoteAccountis byte-for-byte equivalent:_changedAccountsis keyed byHashedKey<Address>(line 31),SetAccountrelies on the same implicit conversion,ContainsKeyis lock-free,TryAddstill guards the race, and hoisting the key reuses the cached hash across both lookups.DeriveTxTypeis order-equivalent to the oldFirstOrDefaultchain._txTypesis built in descendingTxTypeorder (RegisterTransactionType, lines 175–183), soTrailingZeroCountpicks the same winner.type→gasPrice→ discriminator precedence preserved;GasPricecarries no[JsonDiscriminator](onlyAccessList,MaxFeePerGas,MaxPriorityFeePerGas,BlobVersionedHashes,AuthorizationListdo), so skipping the discriminator scan in thegasPricebranch is equivalent. As a bonus it's strictly more robust than the DOM version on anulltransaction body, where the olduntyped.TryGetPropertyValuewould have NRE'd.
Merge recommendation
Not blocked on correctness. Before merge: #10 (amend the test so the GetTrace fix has a guard that can fail). #3 is now optional hardening. #6 is free performance in exactly the scenario this PR targets.
--- · branch perf/eth-call-memory-and-rpc-parse
- CodeInfo: do not latch StreamBuildUnavailable on default CodeHash - GetTrace regression: force rent + 32 KiB dirty buffer past zero chunk - Tx parse: TrySkip, early exit on explicit type, drop dead DiscriminatorProperties, cheaper escaped-name compare, assert <64 types - CodeInfo test: assert no stream is scheduled without a hash Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@claude please re-review — remaining medium/low findings addressed: Medium
Low Ready for another pass. |
|
Claude finished @kamilchodola's task in 4m 35s —— View job Re-review: head
|
| # | Sev | Status |
|---|---|---|
| 1, 2, 4 | — | ✅ fixed earlier (f58ff48, 54e9009) |
| 10 | ✅ fixed — test is no longer vacuous | |
| 3 | ✅ fixed — latch removed | |
| 5 | ✅ TrySkip() |
|
| 6 | ✅ early break |
|
| 7 | ✅ mostly — one alloc per compare remains, acceptable | |
| 8 | Low | Math.Min → Debug.Assert swap regresses Release behaviour |
| 9 | Low | ❌ still passes on master; the new form also mutates a process-wide static |
✅ #10 — the GetTrace test now discriminates
I traced it byte by byte against the current RentSlow:
dirty.TrySave(0, 32 KiB of 0xff)→Rent(32768),_lastZeroedSize = 32768, buffer filled0xff.Dispose()→Returnpushes it at_cachedArrayCount++(32 KiB ≤MaxCachedArrayLength= 64 KiB, so it is cached).memory.TrySaveWord(0, …)→UpdateSize(32)→EnsureRented→RentSlow→Rent(1024).Rentscans top-down and the dirty array is the most recently pushed entry, so it is deterministically the one returned._lastZeroedSize = 0,target = min(32768, roundUp(32, 4096)) = 4096.[0, 4096)zeroed;[4096, 32768)stays0xff.GetTrace()→ClearForTracing(32)no-ops (32 > 4096false),visible = 32.Slice(0, 8192)→8192 > 32→ zero-pad branch → 8192 zeros. ✔
With the clamp reverted, span.Length is 32768, 8192 <= 32768, and the slice returns [4096, 8192) as 0xff → assertion fails. The fix now has a guard that can fail. paddingNeeded is 8160, well under the 1 MiB MemoryPadLimit, so no spurious throw.
Minor robustness note (not worth changing): if the 16-slot thread-local cache were already full when dirty disposes, Return drops the array silently and the test would go vacuous again. Not reachable in practice — these tests hold at most one or two live EvmPooledMemory at a time.
✅ #3 — stateless skip, correct
if (CodeHash == default)
return null;No Volatile.Write, so a CodeInfo executed before StaticCodeCache.Set assigns its hash can still build a stream later. The StreamBuildUnavailable latch is now written only by BuildStream, where it means "this bytecode genuinely can't be streamed" — which is the right semantics for a permanent latch.
✅ #5, #6, #7
if (!reader.TrySkip()) break;— no moreInvalidOperationExceptionon a non-final-block reader.if (setType is not null) break;— I re-confirmedRead()scans a copy (Utf8JsonReader txTypeReader = reader;, line 193) and hands the untouchedreadertoDeserialize, so abandoning the scan mid-object is safe.setTypeshort-circuits ahead of bothhasGasPriceandviaDiscriminatorin the resolution block, so the outcome is unchanged — including theUnknown transaction typethrow.NameEqualsIgnoreCase— the hot path is unchanged and allocation-free; the escaped path now triesValueTextEquals(alloc-free, ordinal) before falling back. Theunescaped.Length != lowerCaseName.LengthUTF-16-vs-UTF-8 length compare is safe because thec > 0x7fcheck rejects everything non-ASCII anyway. OneGetString()per (escaped property × discriminator) remains in the adversarial case, but that's half the old cost and the fold invariant is now documented. Good enough.
Low — the two that aren't closed
#8b — Math.Min(_txTypes.Count, 64) → Debug.Assert regresses Release behaviour (TransactionForRpc.cs:176)
Removing the dead DiscriminatorProperties is right. The bound placement is right too — the assert runs pre-insert, so < 64 before ⇒ <= 64 after ⇒ max index 63, and it's correctly only on the add path.
But Debug.Assert is compiled out of Release while the loop now runs to _txTypes.Count unconditionally. A 65th type would do 1UL << 64, which C# masks to a shift of 0 → sets bit 0 → TrailingZeroCount resolves to _txTypes[0] (SetCodeTransactionForRpc). That's a silent wrong-type deserialization where the old code silently ignored the type. Registration is startup-only, so if (_txTypes.Count >= 64) throw costs nothing and makes it loud.
#9 — the CodeInfo test still passes on master (CodeInfoTests.cs:212)
Re-traced against origin/master's GetOrBuildStream with BuildThreshold = 1: the first call skips the cache (CodeHash != default is false), increments past the threshold, schedules a StreamBuilder, returns null; BuildStream then refuses to Set and latches Unavailable; every later call returns null. So all three assertions hold on the unfixed code. The difference the name promises — no work item scheduled — is _streamBuildState staying Idle, and nothing reads it.
Separately, StreamInterpreter.BuildThreshold is a public static int (VirtualMachine.Stream.cs:26). Nethermind.Evm.Test has [Parallelizable(ParallelScope.All)] fixtures, and NUnit drains the non-parallel queue on the main thread while parallel workers run — so for the duration of this test other EVM tests see a threshold of 1 and take the stream path after one execution. The hashed half also permanently publishes keccak(32× JUMPDEST) into the global InstructionStreamCache. The mutation doesn't even buy anything: the original loop-past-the-default-threshold form had identical discriminating power without touching global state.
Also re-verified this pass
System.Diagnosticsadded forDebug.Assert;System.Text/System.Linqstill genuinely used (Encoding.UTF8.GetBytesat line 163,.Where/.Selectat 157). No new unusedusing.EvmPooledMemoryinvariants unchanged bye1c5c1b— it only touched the test.Read()'s reader-copy discipline,_txTypesdescending-order/TrailingZeroCountequivalence, andSnapshotBundle.PromoteAccountare as previously verified; nothing in this commit disturbs them.
Not run: dotnet is not in this job's allowlist, so I could not compile or execute the tests — everything above is from reading. StreamInterpreter.BuildThreshold is a mutable static field (not const), and EvmPooledMemory.WordSize / CodeInfo.CodeHash are public, so the new test code should compile; please confirm the suite is green locally.
Merge recommendation
Not blocked. Both remaining items are Low and neither affects production behaviour today. #8b is a two-token change worth taking; #9 is a test-hygiene item you can defer.
· branch perf/eth-call-memory-and-rpc-parse
| { | ||
| // Discriminator bitset in DeriveTxType is ulong — keep registration count within it. | ||
| Debug.Assert(_txTypes.Count < 64); | ||
|
|
There was a problem hiding this comment.
Low — swapping Math.Min(_txTypes.Count, 64) for a Debug.Assert turns a silent cap into a silent mis-selection in Release.
The bound placement itself is right: the assert runs before the insert, so < 64 before ⇒ <= 64 after ⇒ max index 63, which 1UL << 63 handles. And it's correctly only on the add path (the replace path doesn't grow the list).
But Debug.Assert is compiled out of Release, and the loop in DeriveTxType now runs to _txTypes.Count unconditionally. With a 65th registered type, 1UL << 64 masks the shift count to 0 in C# and sets bit 0 — so a discriminator belonging to _txTypes[64] would resolve to _txTypes[0] (currently SetCodeTransactionForRpc, the highest-priority entry, and TrailingZeroCount makes bit 0 win outright). Previously that type was simply never matched and the request fell through to the gasPrice/EIP-1559 default.
Not reachable today (5 built-ins + a couple of plugin types), and RegisterTransactionType is startup-only — which is exactly why a hard guard is free here:
| // Discriminator bitset in DeriveTxType is ulong — keep registration count within it. | |
| if (_txTypes.Count >= 64) throw new InvalidOperationException($"At most 64 transaction types can be registered."); |
That converts an invisible wrong-type deserialization into a loud startup failure.
| } | ||
|
|
||
| [Test] | ||
| public void GetOrBuildStream_without_code_hash_never_schedules_build() |
There was a problem hiding this comment.
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:
unhashed.GetOrBuildStream()→CodeHash != defaultis false so the cache lookup is skipped;Interlocked.Increment→ 1;1 < 1is false; the CAS schedules aStreamBuilder; returnsnull.BuildStream()runs,CodeHash != defaultfails, so it takes theelsebranch and never callsInstructionStreamCache.Set.- Every later call returns
null— either from theStreamBuildUnavailablelatch 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.
EVM Opcode Benchmark DiffAggregated runs: base=3, pr=3 No significant regressions or improvements detected. |
Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
CI failures on
Re-ran all failed workflow runs with |
|
Claude finished @kamilchodola's task in 6m 38s —— View job Re-review: head
|
| # | Sev | Status |
|---|---|---|
| 1, 2, 3, 4, 5, 6, 7, 10 | — | ✅ fixed in f58ff48 / 54e9009 / e1c5c1b |
| 11 | Low (new) | ❌ PromoteAccount pre-check is a guaranteed miss on the Get path |
| 8b | Low | ❌ Debug.Assert(_txTypes.Count < 64) is compiled out of Release |
| 9 | Low | ❌ CodeInfo test passes on master and mutates a process-wide static |
✅ 1f3fc9d — EmptyTraceMemory is equivalent
private static readonly TraceMemory EmptyTraceMemory = new(0, default);
public TraceMemory GetTrace()
{
ulong size = Size;
if (size == 0) return EmptyTraceMemory;
ClearForTracing(size);
if (_memory is null) return new(size, default);
int visible = (int)Math.Min(size, (ulong)_memory.Length);
return new(size, _memory.AsMemory(0, visible));
}- The skipped
ClearForTracing(0)was already a no-op (0 > _lastZeroedSizeis false for all unsigned_lastZeroedSize), so the early return changes nothing. - Pre-
1f3fc9dthesize == 0path producednew(0, _memory.AsMemory(0, 0))— an emptyReadOnlyMemorywith size 0, identical toEmptyTraceMemoryfor every consumer (ToHexWordListallocates a zero-length array,ToRawWordBytesshort-circuits onSize == 0,Slice(0, n)takes the pad branch either way). - Keeping
new(size, default)for_memory is null && size > 0is the right call and matchesmaster's behaviour for theCalculateMemoryCost(rentIfNeeded: false)state —ToHexWordListstill emits the zero words rather than an empty array.
Worth noting for the record (your reply to @LukaszRozmej already says this): TraceMemory is a readonly struct, so the old new(...) was never a heap allocation. The static field is a copy from a static base rather than a stack zero-init, so this is neutral, not a win — harmless either way, and GetTrace is off the hot path.
✅ 67712f4 — CodeInfo unchanged in substance
Only the // No cache key … comment was dropped; the stateless if (CodeHash == default) return null; and the absent Volatile.Write are intact. BuildStream remains the only writer of the StreamBuildUnavailable latch, which is the correct semantics for a permanent latch.
Low — new this pass
#11 — PromoteAccount's ContainsKey pre-check is a guaranteed miss on the Get path (SnapshotBundle.cs:476)
PromoteAccount has two callers with opposite profiles:
FlatWorldStateScope.cs:170—promote: !isInCurrentSnapshot, whereisInCurrentSnapshotcame fromDoGetAccount's_changedAccounts.TryGetValue(key, …)(line 91) on the same dictionary. So promote is only reached when absence was just proven, and the new probe is dead weight there.PrewarmerScopeProvider.cs:152→FlatWorldStateScope.cs:186— unconditionalpromote: true. This is where re-promotes of hot accounts skip the bucket lock, so the optimisation does have a real target.
Not a correctness issue (TryAdd still arbitrates the race, and hoisting HashedKey<Address> correctly reuses the cached _hashCode), and the miss costs a lock-free probe on an already-hot cache line. Low.
Low — carried over
#8b — Debug.Assert(_txTypes.Count < 64) (line 175) is compiled out of Release while DeriveTxType now loops to _txTypes.Count unconditionally (line 234, no Math.Min). A 65th registered type would do 1UL << 64, which C# masks to a shift of 0 → sets bit 0 → TrailingZeroCount resolves to _txTypes[0] (SetCodeTransactionForRpc). Silent wrong-type deserialization where the old code silently ignored the type. RegisterTransactionType is startup-only, so if (_txTypes.Count >= 64) throw is free and makes it loud.
#9 — GetOrBuildStream_without_code_hash_never_schedules_build (CodeInfoTests.cs:211) still passes with the CodeHash == default early return removed (on master the threshold-crossing call schedules a build that BuildStream then refuses to publish, so every assertion holds), and it sets the process-wide StreamInterpreter.BuildThreshold to 1. CodeInfoTests is a plain [TestFixture] — the sibling InstructionStreamTests.cs:207 is explicitly [TestFixture, NonParallelizable], which is the precedent — so for the ~250 ms of the negative SpinUntil any concurrently-running parallelizable fixture (Eip7708Tests, KzgPointEvaluationPrecompileTests, CodeInfoRepositoryTests, …) sees a threshold of 1. The hashed half also permanently publishes keccak(32× JUMPDEST) into the global InstructionStreamCache.
Re-verified from source this pass
Not taken on trust from earlier passes, since the master merge could have shifted things (it brought in only 8274eb9, which touches none of these files):
RentSlowchunk-zeroing invariants.target = min(memory.Length, roundUp(Size, 4 KiB)). After the null branchmemory.Length >= Size(Rent(max(Size, 1024))), after the grow branchmemory.Length >= Sizetoo, and in the no-realloc caseSize <= memory.Lengthby theEnsureRentedguard — sotarget >= Sizein all three, preserving_lastZeroedSize >= Sizeon exit.Array.Clear(memory, (int)_lastZeroedSize, (int)(target - _lastZeroedSize))is in bounds because_lastZeroedSize <= memory.Lengthalways (bothRentSlowandClearForTracingcap at it).- The grow branch's
Array.Copy(memory, 0, grown, 0, (int)_lastZeroedSize)copies all live data, since every write path goes throughEnsureRentedwhich zero-extends to the fullSize— so_lastZeroedSizeis always ≥ the written high-water. And it is in bounds: grow requiresSize > memory.Length, so_lastZeroedSize <= memory.Length < Size <= grown.Length. Disposeleaves_lastZeroedSize/Sizestale, but thememory is nullbranch ofRentSlowresets_lastZeroedSize = 0before zero-extending, so a recycledVmStatecan't inherit a bogus zeroed-prefix claim.- Large arrays now come back dirty from the pools.
ArrayPoolcan hand back a bucket larger than requested (e.g. 128 KiB for a 100 KiBSize), leaving[Size, memory.Length)holding unrelated process bytes. The two paths that can read aboveSizeare both covered:GetTraceclamps tovisible = min(Size, _memory.Length), andInspect(line 266) callsClearForTracing(location + length)— which zeroes up tomin(largeSize, _memory.Length)— before handing back the slice. Every EVM-visible read (TrySave*,TryLoad*,*AfterGas,CopyAfterGas,Load32BytesAfterGas) is inside[0, Size). - MCOPY re-checked (
EvmInstructions.Storage.cs:303):UpdateMemoryCost(ref gas, UInt256.Max(b, a), c, …)raisesSizeto covermax(src,dst)+lenbeforeCopyAfterGas, andEnsureRentedzeroes toSize(not to the accessed length), so the source read atbsits inside the zeroed prefix even thoughCopyAfterGasonly callsPrepareAccessAfterGas(dest + len). TheTTracingInst.IsActivebranch'sLoadSpanAfterGas(in b, length)is covered by the same reasoning. - All five
Pooled_buffer_is_zeroed_on_reusecases hand-traced throughRent/RentSlow: 1024 and 4096 go to the thread-local cache and are fully re-zeroed (target == memory.Length); 32 KiB likewise; 70 KiB crossesMaxCachedArrayLengthintoSafeArrayPool.Shared(rented dirty, zeroed toroundUp(71680, 4096) == 71680, and only[0, 71680)is asserted); 2 MiB crossesMaxSharedArrayLengthinto_largeArrayPool. All pass, none regress. GetTrace_slice_past_size_does_not_leak_dirty_bytesstill discriminates on the current file: the 32 KiB0xffarray is the most recently pushed cache entry andRentscans top-down, soRent(1024)deterministically returns it;_lastZeroedSizelands at 4096,[4096, 32768)stays0xff, and with the clamp revertedSlice(0, 8192)returns those bytes.paddingNeededis 8160, well under the 1 MiBMemoryPadLimit.DeriveTxTyperemains order- and precedence-equivalent._txTypesis built in descendingTxTypeorder (lines 177–186), soTrailingZeroCount(discriminated)picks the same winner as the oldFirstOrDefault.type→gasPrice→ discriminator precedence preserved, including theUnknown transaction typethrow.reader.Read()+TrySkip()correctly consumes object/array values; thetypebranch leaves the reader on the value's last token so thecontinuere-enters cleanly.Read()still scans a copy (Utf8JsonReader txTypeReader = reader;), so the earlybreakcannot desync the real deserialize.| 0x20fold re-checked: for an ASCII-letter target the pre-images are exactly{lower, upper}, and every registered discriminator (AccessList,MaxFeePerGas,MaxPriorityFeePerGas,BlobVersionedHashes,AuthorizationList) plustype/gaspriceis pure ASCII letters. Non-ASCII bytes can't collide (0x80+ | 0x20never lands in0x61–0x7A). The constraint is documented on the method.- Usings/lint:
System.Diagnostics(Debug.Assert),System.Numerics(BitOperations),System.Text(Encoding.UTF8.GetBytes) are all used;System.Linqis still needed (.Where/.Selectat 155–156,FirstOrDefaultat 318).System.Text.Json.Nodesis gone. No new unused directive.
Not run: dotnet is not in this job's allowlist, so nothing here was compiled or executed — all of the above is from reading. Your note about the 1f3fc9d CI failures being a GitHub Actions Service Unavailable outage matches the pattern (Code Lint / Code formatting / Build all green, failures clustered on Failed to resolve action download info); worth a glance at the rerun before merging.
Merge recommendation
Not blocked. All Critical/High/Medium findings from the earlier passes are closed. The three remaining items are Low and none affects production behaviour today — #8b is a two-token hardening worth taking, #9 and #11 are deferrable.
· branch perf/eth-call-memory-and-rpc-parse
| { | ||
| // ContainsKey is lock-free; TryAdd alone would take the bucket lock on every hot re-promote. | ||
| HashedKey<Address> key = new(address); | ||
| if (!_changedAccounts.ContainsKey(key)) |
There was a problem hiding this comment.
Low (perf) — on the FlatWorldStateScope.Get path this ContainsKey is a guaranteed miss, so it is pure added work there.
PromoteAccount has exactly two live callers, and they have opposite profiles:
FlatWorldStateScope.cs:170—HintGet(address, account, promote: !isInCurrentSnapshot).isInCurrentSnapshotis set byDoGetAccount(line 91) from_changedAccounts.TryGetValue(key, …)on the same dictionary. Sopromoteis true only when the caller has just proven the key absent, and the newContainsKeyre-lookup will (barring a concurrent insert) always returnfalse— a second hash + probe, thenTryAddtakes the bucket lock anyway.PrewarmerScopeProvider.cs:152→FlatWorldStateScope.cs:186(promote: true, unconditional). This is where the win is: repeated hints for the same hot account skip the bucket lock.
So the optimisation is real, but it's targeted at the prewarmer path only, and it taxes the Get path. Since DoGetAccount already computed HashedKey<Address> key and knows the answer, the cheapest shape is to let the caller that knows skip the probe — e.g. an overload that takes the already-hashed key and a knownAbsent flag, or simply keep TryAdd for the Get path and use the pre-check only on the unconditional HintGet.
Not a correctness issue (TryAdd still arbitrates the race), and the miss cost is a lock-free probe on an already-hot line — hence Low. But if the −2.7% was attributed partly to this, it's worth confirming which caller dominates before assuming the win generalises.
5 participants
Changes
RentSlow) instead of bulk-clearing the high-water mark on frame exit; thread-local buffers are cached dirty and zero-extended just before use.CodeInfo.CodeHashis default (eth_call state overrides / uncacheable code) so per-build buffers are not churned and discarded.TransactionForRpcconcrete subtype by scanning property names onUtf8JsonReaderinstead of materializing aJsonObjectDOM (large eth_call payloads).ContainsKeybeforePromoteAccountTryAddso hot-account re-promotions stay off the concurrent dictionary bucket lock.CodeInfostream skip, large-calldata tx type detection.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
EvmPooledMemoryTestsfor dirty cache reuse (1–32 KiB + large paths) and grow-after-reuse prefix preservation.CodeInfoTests.GetOrBuildStream_without_code_hash_is_unavailable.TransactionForRpcDeserializationTestswith large calldata cases.EvmPooledMemoryTests+CodeInfoTests.GetOrBuildStream*(60) andTransactionForRpcDeserializationTests(59) all green.jsonbench-sweep, ethcallchaos-percategory-validated, 100 RPS / 120s, 3×): paired vs master −2.7% avg, better p95/p99; 497/497 corpus parity.Documentation
Requires documentation update
Requires explanation in Release Notes
Minor eth_call / JSON-RPC path optimizations: lower latency on dense eth_call workloads via deferred EVM memory zeroing, skipping uncacheable code-stream builds, cheaper tx JSON subtype selection, and reduced flat-state promotion lock contention.
Remarks
Product-only extract from
perf/ethcall-497-iter6(no rpc-bench harness changes). Benchmark red on those runs was reth corpus parity vs master, not these branches.