perf(rpc): fix eth_call degradation under concurrent load (50→300 rps) - #12870
perf(rpc): fix eth_call degradation under concurrent load (50→300 rps)#12870kamilchodola wants to merge 10 commits into
Conversation
Per-thread LIFO caches in front of the global VmState and stack pools, coarse-clock eviction tickers in the associative caches, and a test-before-set access mark in ClockCache. Uncontended these ops are nearly free; at 8 concurrent RPC threads each is a serialized cross-core cache-line transfer, thousands of times per eth_call.
…re hints All non-block-processing threads shared one "other" word per counter, so per-node and per-DB-get metric updates were contended cross-core RMWs under concurrent RPC load. StripedLong lands increments on per-core 128-byte-spaced slots. GC.Add/RemoveMemoryPressure now fires only for spans >= 16KB — it mutated GC-global accounting twice per DB read.
Buffers between 64KB and 256KB round-tripped ArrayPool.Shared on every frame, migrating across cores under concurrent RPC load — so the zero- extension stores on frame growth hit cold, coherence-invalidated lines. Retain them in the per-thread cache under a 2MB/thread budget; allocation provenance is unchanged.
…erload-rejection metric Init.InstructionStreamCacheSize replaces the experiment env knob: under concurrent load the hot code set outgrows the 1024-entry cache and frames fall back to the streamless interpreter (measured -25..-40% at 300rps from 4096 on an 8-core box). EthModuleConcurrentInstances now defaults to 2x core count: the caps fail fast and offered concurrency is rate x latency, so at 1x a node serving override-carrying eth_calls rejects whenever latency crosses cores/rate. Rejections are counted so the (deliberately unlogged) shedding is visible to operators.
|
Claude finished @kamilchodola's task in 8m 52s —— View job Review:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // that callers are being shed at a concurrency cap. | ||
| if (errorCode is ErrorCodes.LimitExceeded or ErrorCodes.ModuleTimeout) | ||
| { | ||
| Metrics.JsonRpcOverloadRejections++; |
There was a problem hiding this comment.
High — this counter misses the rejection class the PR is actually about.
ReturnErrorResponse only sees exceptions that escape ExecuteAsync. That covers BoundedModulePool (LimitExceededException / ModuleRentalTimeoutException thrown from await _rpcModuleProvider.Rent(method), outside the inner try), but not the override-environment cap: ShareableOverridableEnvSource throws ConcurrencyLimitReachedException (Nethermind.State/OverridableEnv/ShareableOverridableEnvSource.cs:69) during method invocation, so it is caught by ExecuteAsync's inner catch → HandleInvocationException → the LimitExceeded arm at JsonRpcService.cs:539-542, which returns "Too many requests" without touching the metric.
That is precisely the path the PR description calls out ("an 8-core node serving override-carrying eth_calls rejects with 'Too many requests' … the shedding was invisible"), and the metric's own [Description] claims to cover the "override-environment limit". As written the counter can read 0 while override-carrying eth_calls are being shed.
Simplest fix: move the increment into GetErrorResponse(…) keyed on errorCode, so both paths (and JsonRpcProcessor's batch-limit rejections) are counted from one place.
Also minor: Metrics.JsonRpcOverloadRejections++ is a non-atomic RMW on a static hit concurrently by every RPC thread — it will under-count exactly under the overload it is meant to measure. (It matches JsonRpcErrors++ in the same file, but this counter is specifically for high-concurrency conditions, so Interlocked.Increment would be worth it.)
| // x64/ARM64 hardware. A race with a concurrent Set only affects eviction ranking, | ||
| // not key/value correctness — the "losing" ticker value is simply slightly stale. | ||
| if (TRefreshTicker.IsActive) | ||
| e.Ticker = Interlocked.Increment(ref _ticker); | ||
| { | ||
| long now = Environment.TickCount64; | ||
| if (e.Ticker != now) | ||
| e.Ticker = now; | ||
| } | ||
| value = storedValue; | ||
| return true; |
There was a problem hiding this comment.
Medium — the replacement clock is coarser than the comment implies, and this degrades eviction in the very cache the PR is trying to fix.
Removing the per-hit Interlocked is clearly right. The concern is the choice of replacement source, not the removal.
Environment.TickCount64 is not "millisecond granularity" in practice:
- Windows:
GetTickCount64, which advances at the system timer tick — typically ~15.6 ms. - Linux:
clock_gettime(CLOCK_MONOTONIC_COARSE), i.e. one jiffy — 1–4 ms depending onCONFIG_HZ.
Ticker is only ever consumed by Pick3RandomEvictEntry → Pick3RandomEvict(ta, tb, tc, …), which picks the minimum. Under the load this PR targets (300 rps, hot code set), all 8 ways of a set are touched many times inside a single tick, so all three sampled tickers are equal and 3-random-min degenerates to plain random eviction within the set. Previously Interlocked.Increment gave a strictly increasing, unique value per hit, i.e. a genuine approximate-LRU ordering.
That interacts directly with the other half of this PR: Init.InstructionStreamCacheSize exists because the instruction-stream cache conflict-evicts under concurrency — and this change makes the eviction victim choice within a conflicted set worse at the same time. The benchmark arms can't separate the two, since both landed together.
Suggestions, in rough order of preference:
- Keep a monotonic-but-cheap source with sub-tick resolution, e.g.
Stopwatch.GetTimestamp() >> 16(~µs–tens-of-µs buckets on x64, no shared line, no RDTSC serialization concerns beyond the read itself) — you already payStopwatch.GetTimestamp()on the eviction path below. - Or keep
TickCount64but add a hit-rate measurement forInstructionStreamCache/ the state caches before and after, so the eviction-quality cost is quantified rather than assumed.
Either way, a unit test asserting "under a set-conflicting key stream, the least-recently-read entry is evicted more often than chance" would pin the intended behaviour down. Same comment applies to AssociativeKeyCache.cs:100-112.
| public Task Execute(CancellationToken _) | ||
| { | ||
| // Before any EVM execution: the cache captures this value when its static state initializes. | ||
| Evm.MemoryAllowance.InstructionStreamCacheSize = initConfig.InstructionStreamCacheSize; |
There was a problem hiding this comment.
Medium — unvalidated config reaches a static field initializer, so a typo becomes a runtime TypeInitializationException instead of a startup error.
InstructionStreamCache._cache is private static readonly AssociativeCache<…> _cache = new(MemoryAllowance.InstructionStreamCacheSize); (Nethermind.Evm/CodeAnalysis/InstructionStreamCache.cs:18). With this value now operator-supplied and unchecked:
InstructionStreamCacheSize = 0→AssociativeCache(0)takes themaxCapacity == 0path,_entries = [],_setCount = 0. EveryGetmisses and everySetis a no-op — the instruction-stream cache is silently disabled and every frame runs the streamless interpreter. That's the exact pathology this PR is fixing, delivered by a plausible "turn it off" config value.- Negative value →
ArgumentOutOfRangeException.ThrowIfGreaterThan((uint)maxCapacity, MaxCapacity)(the cast makes-1→4294967295) throws inside the type initializer, so the node starts fine and then dies with aTypeInitializationExceptionat the first EVM execution.
Please validate here (or in InitConfig) — throw a clear ConfigurationException/clamp for values outside [1, AssociativeCache.MaxCapacity] — and log the effective value.
Logging the effective value also addresses the second half of the comment above it: "Before any EVM execution" is an ordering assumption with no assertion behind it. If any step, plugin, or DI factory touches InstructionStreamCache before this step runs (the field is beforefieldinit, so the type initializer fires on first access), the setting is silently ignored and there is no way for an operator to tell. An info-level line here plus a way to read back the cache's actual capacity would make a violation observable.
Design nit (AGENTS.md / di-patterns.md): this introduces a mutable public static crossing an assembly boundary (Init writing into Evm) purely to configure one object. Constructing the cache from the config at its composition point would avoid the global and the ordering hazard entirely — worth considering if it isn't too invasive.
| private static void Return(byte[] array) | ||
| { | ||
| if (array.Length > MaxCachedArrayLength) | ||
| // Provenance: arrays <= MaxNewAllocLength are plain allocations, larger ones came from | ||
| // RentLarge — an array must never reach a pool it was not rented from. | ||
| if (array.Length <= MaxThreadCachedArrayLength | ||
| && _cachedArrayCount < CacheSlots | ||
| && _cachedArrayBytes + array.Length <= MaxThreadCachedBytes) | ||
| { | ||
| ReturnLarge(array); | ||
| byte[]?[] cache = _cachedArrays ??= new byte[CacheSlots][]; | ||
| cache[_cachedArrayCount++] = array; | ||
| _cachedArrayBytes += array.Length; | ||
| return; | ||
| } | ||
|
|
||
| byte[]?[] cache = _cachedArrays ??= new byte[CacheSlots][]; | ||
| if (_cachedArrayCount < CacheSlots) | ||
| if (array.Length > MaxNewAllocLength) | ||
| { | ||
| cache[_cachedArrayCount++] = array; | ||
| ReturnLarge(array); | ||
| } | ||
| } |
There was a problem hiding this comment.
Medium — the provenance invariant holds, but the comment is attached to the branch that doesn't enforce it, and pool-rented buffers are now withheld from the shared pool.
I traced the invariant and it does hold today:
Rentonly allocates fresh whenminLength <= MaxNewAllocLength, andRoundUpToPowerOf2caps that at exactly1 << 16— so no fresh array ever exceedsMaxNewAllocLength.- Therefore the
array.Length > MaxNewAllocLengthguard on theReturnLargebranch can only be reached by arrays that came fromRentLarge. No fresh allocation is ever pushed intoArrayPool, and no pool array is dropped without being returned unless it is parked in the thread cache. EvmPooledMemory.Disposenulls_memorybefore callingReturn, so there's no double-cache path that could hand the same buffer to two frames.
Two things to fix anyway:
-
Comment placement. The
// Provenance: …block sits above the thread-cache branch, which does not check provenance at all; the actual enforcement is thearray.Length > MaxNewAllocLengthtest 8 lines lower. Move it (or restate it) so a future reader who widensMaxNewAllocLengthpastMaxThreadCachedArrayLengthsees the constraint at the place that encodes it. The invariant is entirely implicit in the relationship between three constants — aDebug.Assertor a comment onMaxNewAllocLengthstating "must stay ≤ the round-up ceiling of the fresh-allocation path" would be cheaper to maintain than prose here. -
Buffers in (64 KiB, 256 KiB] rented from
SafeArrayPool<byte>.Sharedare now parked in a[ThreadStatic]cache and never returned while cached. Up toMaxThreadCachedBytes(2 MiB) per EVM-executing thread is borrowed fromArrayPool<byte>.Sharedindefinitely; the pool just allocates replacements for other callers. It is peak-bounded (thread statics are released when the thread dies) so this isn't a leak, but withEthModuleConcurrentInstancesalso doubling in this PR the two changes compound. Worth stating the expected worst case in the comment, and worth a note that the first-fit scan inRentwill happily hand a cached 256 KiB buffer to a 1 KiB request and re-cache it, so one deep call can pin a large buffer to a thread for a long time.
Style nit: MaxThreadCachedBytes = 2 << 20 is correct (2 MiB) but reads inconsistently next to the 1 << N siblings — 1 << 21 would be less error-prone.
| .AddSingleton<IShareableOverridableEnvSource<ReceiptsRegenerationEnv>>(ctx => | ||
| ctx.Resolve<RegeneratingReceiptsEnvSourceFactory>() | ||
| .Create(ctx.Resolve<IJsonRpcConfig>().EthModuleConcurrentInstances ?? Environment.ProcessorCount)) | ||
| .Create(ctx.Resolve<IJsonRpcConfig>().EthModuleConcurrentInstances ?? 2 * Environment.ProcessorCount)) |
There was a problem hiding this comment.
Medium — please confirm doubling receipt-regeneration concurrency is intentional, not just consistency with the eth-module expression.
The comment right above this line says regeneration is "as expensive as executing a block — so it is bounded by the same knob that bounds the eth module itself". Keeping the knob shared is fine; sharing the default is a different tradeoff, because the two consumers have very different per-instance cost. Going from ProcessorCount to 2 * ProcessorCount here doubles the worst-case concurrent block-execution-sized memory for receipt regeneration, and none of the PR's evidence (eth_call corpus, expb block processing) exercises this path.
The PR body justifies the doubling only for eth_call shedding and says "the concurrency default doubles only pool retention that is lazily created under load" — that reasoning is about eth-module instances, not regeneration envs. Either an explicit acknowledgement that the regeneration memory ceiling doubles, or leaving this one at ProcessorCount, would close it out.
| // 16 longs = 128 bytes between live slots. | ||
| private const int SlotStride = 16; | ||
| private static readonly int s_stripeMask = (int)BitOperations.RoundUpToPowerOf2((uint)Environment.ProcessorCount) - 1; | ||
|
|
||
| private readonly long[] _slots = new long[(s_stripeMask + 1) * SlotStride]; | ||
|
|
||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public void Add(long value) |
There was a problem hiding this comment.
Low — a few notes on the new primitive; no correctness problems found.
Add/Sum are correct, Add(-1) works (individual slots go negative, Sum stays right), and the 128-byte stride matches CacheLinePaddedLong. Three small things:
-
Slot 0 is not line-isolated on its low side.
_slots[0]sits at offset 16 in the array object (header + length), so the 64-byte line containing it also contains the array header and whatever object precedes it on the heap. In this PR theStripedLongstatics are allocated back to back, so the preceding object is the previous instance's trailing 128 bytes of padding — safe today, but incidental. Skipping index 0 ((idx + 1) * SlotStride) or padding the head would make it structural. -
Environment.ProcessorCountis captured in astatic readonlyat type-init. With container CPU-limit changes orDOTNET_PROCESSOR_COUNT,Thread.GetCurrentProcessorId()can exceed the mask, which is harmless (two cores share a slot) — worth a sentence in<remarks>so nobody "fixes" it later. -
Memory.
RoundUpToPowerOf2(ProcessorCount) * 128bytes per instance: 16 KiB each on a 96-core box. With ~15 static counters plus 2 perDbOnTheRocksinstance that's a few hundred KiB — fine, but the<remarks>currently only mentions read cost, not allocation size. Worth adding, since the type is now easy to reach for.
There's no unit test for this type. A concurrent-Add-from-N-threads-sums-exactly test plus a negative-Add test would be cheap and would lock in the contract that Sum is exact (not approximate) once writers quiesce — which DbOnTheRocksTests now depends on.
| // Stacks are rented and returned on the executing thread in LIFO order, so a small per-thread | ||
| // cache serves nearly every frame with an array that is hot in this core's cache. The shared | ||
| // queue costs two atomics per frame and migrates ~33 KB pinned arrays between cores under | ||
| // concurrent load; it remains as overflow so deep chains keep pooling and total retention | ||
| // stays bounded. [ThreadStatic] is deliberately shared across pool instances: every pool | ||
| // deals in identically-shaped arrays. | ||
| private const int MaxStacksCachedPerThread = 16; | ||
| [ThreadStatic] private static byte[]?[]? _threadStacks; | ||
| [ThreadStatic] private static int _threadStackCount; | ||
|
|
||
| public partial void ReturnStacks(byte[] dataStack) | ||
| { | ||
| byte[]?[] threadStacks = _threadStacks ??= new byte[]?[MaxStacksCachedPerThread]; | ||
| int cached = _threadStackCount; | ||
| if (cached < MaxStacksCachedPerThread) | ||
| { | ||
| threadStacks[cached] = dataStack; | ||
| _threadStackCount = cached + 1; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Low — cross-instance sharing checks out; two smaller points.
I verified the load-bearing assumption in the comment: StackPool.StackLength is a const ((EvmStack.MaxStackSize + EvmStack.RegisterLength) * 32 = 32,832, plus Vector256<byte>.Count), so every pool instance really does deal in identically-shaped arrays and the [ThreadStatic] sharing is safe. Good that it's called out explicitly.
-
Total retention is no longer bounded by
MaxStacksPooled. The cap wasMaxCallDepth * 2= 2048 arrays ≈ 67 MB. Now it's that plus up to 16 × ~32 KB ≈ 526 KB of pinned (POH) arrays per thread that has executed EVM. It's peak-bounded — thread statics are freed when a thread-pool thread retires — and in practice a shalloweth_callonly parks 1–2 stacks, so the realistic figure is much lower. But POH is not compacted, so thread-pool churn under sustained RPC load can fragment it over time. Since this PR also doublesEthModuleConcurrentInstances, it would be good to state the intended ceiling in the comment, and to say why 16 (rather than, say, 4) is the right per-thread depth — the benchmark evidence doesn't distinguish. -
ReturnStacksallocates the 16-slot array via??=before checkingcached < MaxStacksCachedPerThread, so a thread whose first return overflows still allocates it. Trivial, but the check could come first.
No test covers the new path. A test that rents/returns across two StackPool instances on one thread and asserts the returned array length is cheap insurance for the const-shape assumption above.
The benchmark evidence in this PR was measured at 4096; shipping a 1024 default would not reproduce it. Worst-case retention is size x 512 KiB, but typical streams are far smaller; the frugal 1024 static default remains for hosts that never run node init.
EXPB Benchmark ComparisonRun: View workflow run superblocksScenario: Client Processing (SSE)
K6 TTFB
realblocksScenario: Client Processing (SSE)
K6 TTFB
fusakaScenario: Client Processing (SSE)
K6 TTFB
|
… validate cache size, add regression tests - JsonRpcOverloadRejections moves to the GetErrorResponse funnel so the override-environment cap (thrown during invocation) is counted too; regression test covers both paths. - Cache eviction age uses Stopwatch.GetTimestamp() — TickCount64's jiffy granularity tied hot sets under load, degrading 3-random eviction to random; a deterministic recency test pins the semantics. - Init.InstructionStreamCacheSize validated at startup (0 disabled the cache silently; negatives died later as TypeInitializationException) and the effective value is logged. - Receipts regeneration keeps the 1x-core default (heavier envs, no measured need for the eth module's new 2x headroom). - Tests: StripedLong exactness/negatives, StackPool cross-instance and distinctness, EvmPooledMemory pooling-boundary recycling, plus low-fix cleanups (slot-0 isolation, constants, comment placement).
|
@claude re-review All findings from the first review are addressed in 94e5925:
|
|
Claude finished @kamilchodola's task in 11m 52s —— View job Re-review:
|
| # | Claim | Verdict |
|---|---|---|
| High 1 | metric counts both paths | Fixed. ConcurrencyLimitReachedException → HandleInvocationException → JsonRpcService.cs:536 → GetErrorResponse, and the rental path arrives via ReturnErrorResponse. Both now increment, atomically. Test present. |
| Medium 2 | fine-grained eviction age | Fixed. Ticker is only ever consumed by Pick3RandomEvictEntry's relative min and cleared entries reset to 0, so the unit change carries no absolute-time semantics. Test is deterministic by construction (Pick3Indices returns three distinct ways; the hot entry always holds the set's max ticker), not by luck. |
| Medium 3 | config validated + logged | Fixed for <= 0. |
| Medium 4 | regeneration concurrency | Fixed — back to ProcessorCount with the rationale in a comment. |
| Medium 5 | tests | Fixed. StripedLong (exactness under 2.4 M concurrent mixed adds, negatives, zero), StackPool (cross-instance thread-cache flow + distinctness across 24 rents), EvmPooledMemory (6 cases bracketing 64 KiB / 256 KiB, asserting a deliberately dirtied buffer comes back reading zero — the load-bearing property), the cache recency test, and the metric regression test. Checked the plumbing compiles: InternalsVisibleTo("Nethermind.Evm.Test") exists, StackPool.StackLength is a public const, TestRequestWithPool + NSubstitute.ExceptionExtensions are already in scope, ConcurrencyLimitReachedException is in Nethermind.Core.Exceptions. |
| Lows | all six | Fixed. Verified StripedLong's lead pad is coherent end to end: Add writes (idx+1)*16, Sum iterates stripes 0..mask reading (stripe+1)*16, array is (mask+2)*16 — in bounds, no slot skipped, no slot double-counted. |
Also re-checked, unchanged and still correct: DangerousReleaseMemory's GC-pressure pairing is symmetric on the same threshold and the same span, and thresholding is behaviourally safe because DbSpanMemoryManager releases deterministically (no finalizer) — the pressure hint never drove the free. ClockCacheBase.MarkAccessed's test-before-Or, the VmState/StackPool/EvmPooledMemory provenance and clearing invariants, and StripedLong's exact-sum contract all hold.
Findings
Medium
EthModuleConcurrentInstances' description still says "Defaults to the number of logical processors" —IJsonRpcConfig.cs:159-160. It is now 2× for the eth module and deliberately 1× for receipt regeneration, so the single operator-facing text for the knob is wrong for both consumers, and it is the text that feeds the generated docs. One-line fix; inline. Fix this →
Low (all inline, all cheap)
- The metric funnel is wider than the metric's description —
JsonRpcService.cs:923. Batch-size limit,MaxBatchResponseBodySize, and HTTP 413 also pass throughGetErrorResponsewithLimitExceeded, so the counter can be nonzero with no concurrency shedding at all, while its[Description]advises raisingEthModuleConcurrentInstances. All three concurrency sites passsuppressWarning: trueand none of these do, so that flag isolates them exactly — or just reword the description. (Confirmed theResultWrapperLimitExceededfailures such asMaxLogsPerResponseare not affected: they bypassGetErrorResponse.) - Metric is now a
public staticfield. It exports fine —MetricsControllerwalksGetProperties().Concat(GetFields())— but the repo's pattern for an interlocked counter is a get-only property over a private field (Nethermind.Db/Metrics.cs:FullPruningCount => _fullPruningCount), which also keeps it insideMetricsTests.ValidateMetricsDescriptions(property-only walk). InstructionStreamCacheSizeupper bound unguarded — aboveAssociativeCache.MaxCapacity(1u << 27) the node still starts and then dies withTypeInitializationExceptionat the first EVM execution. Plus: the error text saysIInitConfig.…where the operator's key isInit.…, and the effective-value log is atDebug, so at default log level the ordering assumption is still unobservable.AssociativeKeyCachehas the same ticker change but no test — the neighbouring test in that base class already drives both caches side by side, so extending is a few lines.- The metric test asserts
>= before + 2on a process-global counter, so it cannot distinguish "both paths counted once" from "one path counted twice by a concurrent test". Capturing the delta after each of the two requests separately makes it a real regression test for the pair. - Unverified question:
Metrics.csinNethermind.Evm/Db/Trieis common (not.std.cs), so the ZisK guest build now compilesStripedLong—Thread.GetCurrentProcessorId()is the only use of that API in the repo. The guest is single-threaded so it is semantically harmless; worth a sanity check that the API is supported there, or move the striped counters behind the.stdsplit.
Verdict
The substantive concerns from round 1 are closed, and the fixes are the ones I'd have written — particularly the eviction-age change, which is now both correct in its unit and pinned by a test that is deterministic rather than probabilistic. What remains is text-vs-behaviour: a stale documented default (finding 1) and a metric whose description over-promises (finding 2). I'd fix those two plus the Info log line before merge; the rest can ride.
Not verified: dotnet build and dotnet test are outside my allowed tools in this environment, so this is again a static review. I checked compile-level plumbing for every new test by hand (visibility, InternalsVisibleTo, existing helpers, usings, namespaces) but did not compile or run anything.
· branch perf/ethcall-concurrency
| // rejections reach this point along two distinct paths (module rental before invocation, | ||
| // and the override-environment cap during invocation), and their warnings are suppressed | ||
| // by design — without a counter operators cannot see that callers are being shed. | ||
| if (errorCode is ErrorCodes.LimitExceeded or ErrorCodes.ModuleTimeout) |
There was a problem hiding this comment.
Low — the funnel fixed the miss but widened the counter past its own description.
Both shedding paths now land here, which is the fix I asked for — ConcurrencyLimitReachedException reaches HandleInvocationException → line 536 → this method, and the rental path arrives via ReturnErrorResponse. Interlocked.Increment is right too. Verified that ResultWrapper-based LimitExceeded failures (e.g. EthRpcModule.cs:1103, MaxLogsPerResponse) do not pass through here — they are JsonRpcResponses returned via WithResponseContext, so they are correctly excluded.
Three sites that are not concurrency caps do pass through, though:
JsonRpcProcessor.cs:804— batch size limit exceededJsonRpcProcessor.cs:811(CreateBatchResponseLimitEntry) —MaxBatchResponseBodySizeexceededRunner/JsonRpc/Startup.cs:504— HTTP 413 payload too large
So a node that only ever gets oversized batches reports JsonRpcOverloadRejections > 0, and the metric's [Description] then tells the operator to raise EthModuleConcurrentInstances, which will not help. Either reword the description to "rejected at a request/concurrency limit", or narrow the predicate — all three concurrency sites pass suppressWarning: true and none of the three above do, so errorCode is … && suppressWarning isolates exactly the shedding paths.
Separately: Metrics.JsonRpcOverloadRejections is now a public static field. That works (MetricsController.RegisterMetrics walks GetProperties().Concat(GetFields())), but the established pattern for an interlocked counter in this repo is a get-only property over a private field — e.g. Nethermind.Db/Metrics.cs: public static long FullPruningCount => _fullPruningCount;. That form also keeps it inside MetricsTests.ValidateMetricsDescriptions, which only enumerates properties.
| // Before any EVM execution: the cache captures this value when its static state initializes. | ||
| Evm.MemoryAllowance.InstructionStreamCacheSize = initConfig.InstructionStreamCacheSize; | ||
| ILogger logger = logManager.GetClassLogger<ApplyMemoryHint>(); | ||
| if (logger.IsDebug) logger.Debug($"Instruction stream cache size: {initConfig.InstructionStreamCacheSize} entries"); |
There was a problem hiding this comment.
Low — validation closes the two failure modes I flagged; three small residuals.
The <= 0 guard removes both the silent no-cache and the negative-value TypeInitializationException. Remaining:
- Upper bound still unguarded.
AssociativeCache's ctor doesArgumentOutOfRangeException.ThrowIfGreaterThan((uint)maxCapacity, MaxCapacity)withMaxCapacity = 1u << 27(SeqlockHeader.cs:61). A value above that (a fat-fingered1000000000) still starts the node cleanly and then dies withTypeInitializationExceptionat the first EVM execution — the same failure mode, just at the other end.is <= 0 or > 1 << 27costs nothing here. nameof(IInitConfig)renders asIInitConfig.InstructionStreamCacheSize, but the key an operator sets isInit.InstructionStreamCacheSize(orNETHERMIND_INITCONFIG_INSTRUCTIONSTREAMCACHESIZE). Spelling the config key literally makes the message directly actionable.IsDebughides the effective value at the default log level. The reason for logging it was that "before any EVM execution" is an unasserted ordering assumption with a silent failure mode — if anything touchesInstructionStreamCachebefore this step, the setting is ignored and nothing says so. AtDebugan operator will never see it. OneInfoline at startup, alongside the other memory-hint output, is what makes the assumption observable.
| if (TRefreshTicker.IsActive) | ||
| e.Ticker = Interlocked.Increment(ref _ticker); | ||
| e.Ticker = Stopwatch.GetTimestamp(); | ||
| return true; |
There was a problem hiding this comment.
Resolved (Medium 2) — with one coverage gap.
Stopwatch.GetTimestamp() is the right source here: I checked that Ticker is consumed only by Pick3RandomEvictEntry's relative min, and that cleared entries reset to 0 (which sorts as oldest), so switching the unit carries no absolute-time semantics. Folding the eviction path's second GetTimestamp() into the single timestamp local is a nice side-effect.
Gap: AssociativeKeyCache got the identical change, but Refreshed_entry_survives_sustained_churn_in_its_set only exercises AssociativeCache. The neighbouring test in the same base class already drives both cache and keyCache side by side, so extending it is a few lines and would cover both implementations of the semantics being pinned.
Two notes on the test itself, neither blocking:
- It is deterministic by construction rather than by luck —
Pick3Indicesguarantees three distinct ways, and the hot entry always holds the set's maximum ticker, so the sampled minimum can never be it. Worth knowing the one theoretical hole:Pick3RandomEvictbreaks ties toward the earlier index (ta <= tb), so a tie between the hot entry and both other sampled entries could evict it. That needs three iterations inside one clock tick — impossible on Linux (Stopwatch.Frequency == 1e9), and vanishingly unlikely on Windows QPC's 100 ns tick. - The per-hit clock read (~15–25 ns via vDSO / QPC) is now on the hit path of the hottest users —
StaticCodeCache,InstructionStreamCache,TxPool.HashCache. Those fire per call frame / per tx rather than per opcode, and the expb arms (AVG −1.5% … −3.1%) show no block-processing regression, so this looks paid for; just flagging that it is the new steady-state cost of a hit.
| // eth_calls starts rejecting with "Too many requests" the moment average latency | ||
| // crosses ~(cores / rate) — a cliff a transient latency wobble is enough to trip. | ||
| .RegisterBoundedJsonRpcModule<IEthRpcModule, EthModuleFactory>(jsonRpcConfig.EthModuleConcurrentInstances ?? 2 * Environment.ProcessorCount, jsonRpcConfig.Timeout) | ||
| .AddSingleton<IBlockchainBridgeFactory, ISimulateReadOnlyBlocksProcessingEnvFactory, IOverridableEnvFactory, ILifetimeScope>( |
There was a problem hiding this comment.
Medium — the knob's own documentation still states the old default.
IJsonRpcConfig.EthModuleConcurrentInstances ends with "Defaults to the number of logical processors." (IJsonRpcConfig.cs:159-160). After this PR the eth module defaults to twice that, and ReceiptRegenerationModule deliberately stays at 1× — so the one operator-facing description for the knob is now wrong for both consumers. This is the config-item text that feeds the generated docs, and the PR ticks "requires documentation update" on the strength of config items carrying their own docs.
Suggest: "Defaults to twice the number of logical processors." plus a clause noting that the receipt-regeneration env pool bounded by the same knob defaults to 1× — that asymmetry is otherwise only discoverable by reading two DI modules.
…, bound the cache config - EthModuleConcurrentInstances description states the 2x eth-module / 1x receipt-regeneration split. - Overload counter keys on suppressWarning so batch/body-size LimitExceeded responses are not counted as concurrency shedding, and follows the get-only-property-over-field metric pattern. - Init.InstructionStreamCacheSize gets an upper sanity bound, an operator-facing key name in the error, and an Info-level effective- value log. - Recency test drives AssociativeKeyCache alongside AssociativeCache; metric regression test asserts per-path deltas.
|
@claude re-review Round-2 findings addressed:
|
|
Claude finished @kamilchodola's task in 5m 43s —— View job Re-review (round 3):
|
| # | Claim | Verdict |
|---|---|---|
| Medium 1 | description states both defaults | Fixed. Verified against all three consumers: RpcModules.cs:89 (module pool) and RpcModules.cs:93 (override-path env pool) → 2 * ProcessorCount; ReceiptRegenerationModule.cs:42 → ProcessorCount. Text matches. Wording nit inline. |
| Low 2 | counter keys on suppressWarning |
Fixed, and it is exact. suppressWarning: true is reachable from exactly two places, both concurrency shedding: JsonRpcService.cs:82-84 (LimitExceededException/ConcurrencyLimitReachedException/ModuleRentalTimeoutException → ReturnErrorResponse) and JsonRpcService.cs:535 (HandleInvocationException). The three false positives I named — JsonRpcProcessor.cs:804, :811, Startup.cs:504 — all enter via the public GetErrorResponse(int, string, …) overloads at :902/:905, which never set the flag, so they no longer count. |
| Low 3 | metric follows the property pattern | Fixed. public static long JsonRpcOverloadRejections => _jsonRpcOverloadRejections; + internal static void IncrementJsonRpcOverloadRejections(). Verified it still exports and still can't break registration: MetricsController.TryCreateMetricUpdater only calls memberInfo.SetValue for IMetricObserver members and DetailedMetricOnFlag properties — a long [CounterMetric] takes the GetValueAccessor<double>() path, which is read-only. And MetricsTests.ValidateMetricsDescriptions walks GetProperties(Static | Public), so it now covers this metric (the [Description] is present). |
| Low 4 | upper bound / key name / log level | Fixed as described; see the inline note on how much the 1 << 24 ceiling actually buys. |
| Low 5 | AssociativeKeyCache covered |
Fixed. Checked the structures are actually equivalent — AssociativeKeyCache is Ways = 8, same (int)hashCode & _setMask set selection, same Pick3RandomEvictEntry, and Get is the ticker-refreshing GetCore<OnFlag> — so the test is deterministic for the key cache by the same construction, not by luck. |
| Low 6 | per-path deltas | Fixed. beforeInvocation → assert after path 1 → re-capture beforeRental → assert after path 2. Double-counting one path can no longer masquerade as both being counted. |
| Low 7 | ZisK | Accepted, and I can add the missing half of the argument: even if the guest runtime returns -1 from GetCurrentProcessorId(), (-1 & s_stripeMask) + 1 lands on the top slot, so the index stays in bounds and Sum stays exact — the only residual failure is a hard PlatformNotSupportedException. Confirmed the split mechanism is Directory.Build.targets:8 (EnableZkEvm removes **/*.std.cs), so Nethermind.Evm/Metrics.cs and StripedLong are indeed compiled for the guest; Interlocked is already used there, and [ThreadStatic] is aliased to NoopThreadStaticAttribute for that build, which is what makes the new VmState/EvmPooledMemory thread-caches degrade to plain statics safely in a single-threaded guest. Not worth forking three Metrics files. |
Also re-verified in this pass, unchanged and correct: StackPool.RentStacks nulls the cache slot on rent (no stale ~33 KB retention), the ??=-after-capacity-check reordering, VmState.Return's per-closed-generic [ThreadStatic] pool (allocating the array before the count check there is unreachable-when-full, so harmless), and ClockCacheBase.MarkAccessed's test-before-Or.
Findings
Medium
- The PR body states the wrong default for the knob it introduces. The body says "
Init.InstructionStreamCacheSizeconfig (default 1024, unchanged)", and the release-note line reads "Init.InstructionStreamCacheSize=4096recommended for heavy simulation workloads" — both imply 4096 is opt-in. ButInitConfig.cs:46ships= 4096andIInitConfig.csdeclaresDefaultValue = "4096"(commitc6f0aa1, after the body was written). So the shipped default is 4096, which by the body's own accounting (size × 512 KiBworst case) moves worst-case stream retention from 512 MiB to 2 GiB, and makes "both knob defaults stay memory-conservative" the wrong summary of what merges. The change itself is well evidenced (the same-build A/B: avg −29.5%, p95 −45.9%, failures 1.76% → 0.35%) and I am not arguing against it — but it is now the single largest default-memory change in the PR and it is the one thing the body tells reviewers and release notes did not happen. Please update the body's bullet, the "Remarks" retention sentence, and the release-note line.
Low (all inline)
1 << 24closes the> MaxCapacitycase but is still ~940 MB of eagerly-allocated_entriesinsideInstructionStreamCache's static initializer, so theTypeInitializationException-at-first-execution mode survives at the top of the accepted range;1 << 18–1 << 20would make the message the real guard. —ApplyMemoryHint.cs:33Stopwatch.GetTimestamp()(my own round-2 suggestion) is only vDSO-cheap when the host clocksource is TSC; on a VM fallen back toxen/hpet/acpi_pmit is hundreds of ns to µs, now paid per cache hit onStaticCodeCache/InstructionStreamCache/TxPool.HashCache. Recording the tradeoff, plus the fact that the comment this replaced documented the opposite measurement — a clause reconciling the two would stop it flipping back. —AssociativeCache.cs:172- "one-times" reads awkwardly in generated config docs; suggestion inline. —
IJsonRpcConfig.cs:159-162 - Keying the counter on
suppressWarningis exactly right today, but it couples an observability counter to a logging flag: a futuresuppressWarning: trueon a non-concurrency error silently inflates the metric. The comment at the increment site partly covers this; a note on the two switch arms would close it.
Verdict
The code is in good shape — every substantive concern from rounds 1 and 2 is closed, and the fixes are the ones I would have written. The one thing I would not merge without is finding 1: the description and release-note text disagree with the shipped default on a memory-affecting knob. That is a two-minute edit to the PR body, not a code change.
Not verified: dotnet build/dotnet test are outside my allowed tools in this environment (the build command was denied), so this remains a static review. I hand-checked the compile-level plumbing for the changed test and metric code (member visibility, MetricsController's reflection paths, NUnit's Assert.EnterMultipleScope already in use at HistoryPrunerTests.cs:136, the EnableZkEvm compile-item split) but did not compile or execute anything.
· branch perf/ethcall-concurrency
| // size exists to avoid) and an out-of-range value would surface later as an opaque | ||
| // TypeInitializationException at the first EVM execution, so reject both here. | ||
| // The ceiling is a sanity bound well under the cache's structural capacity limit. | ||
| const int maxInstructionStreamCacheSize = 1 << 24; |
There was a problem hiding this comment.
Low — the bound closes the > MaxCapacity case but not the failure mode behind it.
1 << 24 is well under AssociativeCache.MaxCapacity (1u << 27), so the ArgumentOutOfRangeException-inside-the-type-initializer path is gone. What remains is that 1 << 24 is still far above anything a node can survive:
InstructionStreamCacheisAssociativeCache<ValueHash256, InstructionStream>, so an entry is header (8) +ValueHash256(32) + object ref (8) + ticker (8) ≈ 56 B, and_entriesis allocated eagerly in the ctor. At1 << 24that is ~940 MB plus aint[1 << 21]gate array — allocated insideInstructionStreamCache's static initializer, i.e. still anOutOfMemoryExceptionwrapped inTypeInitializationExceptionat the first EVM execution on most hosts.- The config's own doc says worst-case retention is
size × 512 KiB. At1 << 24that is 8 TiB; even1 << 18is already 128 GiB. So the operator-meaningful ceiling is orders of magnitude below the structural one.
A bound in the 1 << 18 … 1 << 20 range would make the error message the actual guard rather than a formality. Init. prefix in the message and the Info log are both right now.
| overrides: calls beyond this cap fail with a `LimitExceeded` JSON-RPC error. Defaults | ||
| to the number of logical processors. | ||
| to twice the number of logical processors for the eth module and override-path pools | ||
| (offered concurrency is request rate times call latency, so one-times rejects under | ||
| transient latency wobbles); receipt regeneration deliberately stays at one-times. |
There was a problem hiding this comment.
Resolved (round-2 Medium 1) — with a wording nit.
Verified against the three consumers: RpcModules.cs:89 (module pool) and RpcModules.cs:93 (override-path env pool) both default to 2 * ProcessorCount, ReceiptRegenerationModule.cs:42 stays at ProcessorCount. So "the eth module and override-path pools" is accurate and the asymmetry is now discoverable from the one operator-facing text.
Nit: "one-times" reads awkwardly twice ("so one-times rejects…", "stays at one-times"). Since this string is rendered verbatim into the generated config docs, something like "one per logical processor" / "stays at one per logical processor" would read better.
| overrides: calls beyond this cap fail with a `LimitExceeded` JSON-RPC error. Defaults | |
| to the number of logical processors. | |
| to twice the number of logical processors for the eth module and override-path pools | |
| (offered concurrency is request rate times call latency, so one-times rejects under | |
| transient latency wobbles); receipt regeneration deliberately stays at one-times. | |
| to twice the number of logical processors for the eth module and override-path pools | |
| (offered concurrency is request rate times call latency, so one per logical processor | |
| rejects under transient latency wobbles); receipt regeneration deliberately stays at | |
| one per logical processor. |
| // not key/value correctness — the "losing" ticker value is simply slightly stale. | ||
| if (TRefreshTicker.IsActive) | ||
| e.Ticker = Interlocked.Increment(ref _ticker); | ||
| e.Ticker = Stopwatch.GetTimestamp(); |
There was a problem hiding this comment.
Low — one residual risk I should have named when I suggested this source.
The change is the one I asked for and the reasoning in the new comment is right. Two things worth recording, since the comment this replaced documented the opposite conclusion ("Interlocked.Increment is faster than Stopwatch.GetTimestamp() (RDTSC) — ~7.6 ns vs ~19 ns single-threaded, and scales better under contention (20% faster at 8 threads)"):
Stopwatch.GetTimestamp()is only cheap when the host's clocksource is TSC. On Linux it isclock_gettime(CLOCK_MONOTONIC); that resolves in the vDSO without a syscall only fortsc-family clocksources. On a VM that has fallen back toxen/hpet/acpi_pm(older Xen-based instances, some nested-virt setups), the same call becomes an MMIO read or a real syscall in the hundreds-of-ns-to-µs range. That would now be paid on every hit ofStaticCodeCache/InstructionStreamCache/TxPool.HashCache— per call frame and per tx. Your benchmark box is presumably TSC, so the arms cannot see this.- Because the removed comment recorded a measured decision in the other direction, a future reader has two contradictory claims to reconcile. One clause noting that the earlier number compared the two clocks in isolation, while the cost being removed here is the shared-line RMW next to
_epochAndCount, would stop this flipping back.
If (1) is a concern for the node's supported hosts, the cheapest alternative that keeps the old counter's measured single-threaded cost is to keep Interlocked.Increment but move _ticker onto its own cache line (it was adjacent to _epochAndCount, which is the false-sharing cause the comment names) — that addresses the stated mechanism without introducing a clock read on the hit path. Not asking for a change, just recording the tradeoff.
EVM Opcode Benchmark DiffAggregated runs: base=1, pr=1 Improvements (1)
|
… shedding marker Ceiling drops to 1<<20 so the config error, not a type-initializer failure, guards the top of the range; the eviction-clock comment records both sides of the Interlocked-vs-clock measurement so it does not flip back; the suppressWarning switch arms note their metric coupling; config prose drops the awkward wording.
|
@claude re-review Round-3 items addressed:
|
|
Claude finished @kamilchodola's task in 4m 8s —— View job Re-review (round 4):
|
| # | Claim | Verdict |
|---|---|---|
| Medium 1 | body states the shipped default | Fixed in all three places. Changes bullet now reads "default 4096 (was a hardcoded 1024 — the largest default-memory change in this PR)"; the isolation A/B line is labelled "this is the PR's shipped default; the tables above include it"; the release-note line and Remarks both carry the 512 MiB → 2 GiB worst-case accounting with the "typical is far smaller" caveat and the lower-it-on-constrained-setups instruction. Matches the code: InitConfig.cs:46 = 4096, IInitConfig.cs:113 DefaultValue = "4096". |
| Low 2 | ceiling is the real guard | Fixed. 1 << 20 → ~56 MB eager entry table (Entry = 8 + 8 + 32 + 8), so the validation message, not an OOM inside InstructionStreamCache's type initializer, is what an operator hits anywhere in the accepted range. Notes inline. |
| Low 3 | clocksource tradeoff recorded | Fixed, and it reconciles the two measurements rather than just replacing one. The comment now states that the clock read loses single-threaded (and more on non-TSC hosts) while winning under concurrent readers — "the regime that motivated the change" — plus an explicit "do not flip back to a shared counter for the ns." Applied identically in AssociativeCache.cs and AssociativeKeyCache.cs. |
| Low 4 | config prose | Fixed. "one-times" is gone; "a processor-count cap" reads correctly in generated docs, and the receipt-regeneration asymmetry now carries its rationale ("as expensive as block execution") in the operator-facing text, not just in the DI module. |
| Low 5 | shedding-marker coupling noted | Fixed at both suppressWarning: true sites (JsonRpcService.cs:83 switch, :534 HandleInvocationException arm). Re-confirmed the coupling is still exact: suppressWarning: true reaches the private GetErrorResponse from only those two paths, both concurrency shedding; ResultWrapper.cs:83 sets Error.SuppressWarning directly and bypasses the funnel, so IsTemporary errors (Taiko, MaxLogsPerResponse) cannot inflate the counter. |
Also re-verified in this pass, unchanged and correct: StripedLong's lead-pad arithmetic is coherent end to end (Add writes (idx+1)*16, Sum reads stripes 0..mask at (stripe+1)*16, array is (mask+2)*16 — in bounds, no slot skipped or double-counted) and the <remarks> allocation formula (stripes + 1) * 128 matches; every Metrics.cs converted to StripedLong keeps Interlocked in use so no using went unused (lint-clean) and all four already imported Nethermind.Core.Threading; the metric is still export-safe and still inside MetricsTests.ValidateMetricsDescriptions' property walk; the new test's unqualified Metrics resolves to Nethermind.JsonRpc.Metrics via the enclosing-namespace lookup, ahead of the Nethermind.Trie / Nethermind.Evm compilation-unit usings that also declare a Metrics, so there is no ambiguity there.
Findings
Low (both inline, both on the new config guard, neither blocking)
InvalidDataExceptionwhere the repo has a dedicated config type —ApplyMemoryHint.cs:37.InvalidConfigurationExceptioncarries an exit code viaIExceptionWithExitCode(Program.cs:89,:252) and is what the newer validation in this assembly uses (InitializeNetwork.cs:127,PruningTrieStateFactory.cs:196,NethermindModule.cs:124);InvalidDataExceptionmatches onlyMemoryHintMan's older style. Behaviourally identical here — same log, sameExitCodes.GeneralError— so purely consistency. Also: no test pins the accepted range.- The
[1, 1 << 20]range is enforced but not documented —IInitConfig.cs:113. TheConfigItemdescription documents units and default percoding-style.md; one clause naming the ceiling would stop an operator discovering it via a node that refuses to start.
Verdict
Ready to merge from my side. Four rounds in, every substantive concern is closed in the code, and the two text-vs-behaviour gaps from round 3 (the shipped 4096 default in the body, the awkward config prose) are now consistent with what ships. The remaining Lows are wording and exception-type consistency; neither changes behaviour.
Not verified: dotnet build/dotnet test are outside my allowed tools in this environment (git fetch was denied too, so I diffed against the PR's merge-base commit 219d999 in the local history rather than origin/master). This remains a static review — I hand-checked compile-level plumbing for the changed code (member visibility, using liveness after the metric rewrites, name resolution for the new test's Metrics reference, MetricsController's reflection paths) but did not compile or execute anything.
· branch perf/ethcall-concurrency
| const int maxInstructionStreamCacheSize = 1 << 20; | ||
| if (initConfig.InstructionStreamCacheSize is <= 0 or > maxInstructionStreamCacheSize) | ||
| { | ||
| throw new InvalidDataException( | ||
| $"Init.{nameof(IInitConfig.InstructionStreamCacheSize)} must be between 1 and {maxInstructionStreamCacheSize}, got {initConfig.InstructionStreamCacheSize}."); |
There was a problem hiding this comment.
Resolved (round-3 Low 2) — with two cosmetic notes.
1 << 20 is the right end of the range I suggested: AssociativeCache<ValueHash256, InstructionStream>'s Entry is Header(8) + Ticker(8) + ValueHash256(32) + ref(8) = 56 B, so the eagerly-allocated table tops out at ~56 MB — the comment's "tens of MB" is accurate, and this validation is now genuinely the guard across the whole accepted range rather than a formality. Init. prefix and the Info log are both right.
Two nits, neither blocking:
InvalidDataExceptionvsInvalidConfigurationException. The repo has a dedicated config-failure type that carries an exit code (Nethermind.Core/Exceptions/InvalidConfigurationException.cs, consumed byProgram.cs:89/:252viaIExceptionWithExitCode), and the newer validation in this assembly uses it —InitializeNetwork.cs:127,PruningTrieStateFactory.cs:196,NethermindModule.cs:124.InvalidDataExceptionmatches onlyMemoryHintMan's older style. Behaviourally identical here (both surface as "unhandled error" +ExitCodes.GeneralError), so purely a semantic/consistency point.- No test for the guard. Two
Assert.Throwscases onExecute(0 and(1 << 20) + 1) would pin the range that the config docs now imply. Optional given how much test coverage the rest of the PR gained.
| DefaultValue = "4096")] | ||
| int InstructionStreamCacheSize { get; set; } |
There was a problem hiding this comment.
Low — the accepted range is now enforced but not documented.
ApplyMemoryHint.cs:34-38 rejects anything outside [1, 1 << 20] at startup, and this [ConfigItem] description is the only operator-facing text for the key. Units and default are documented (per coding-style.md), but the bound is discoverable only by tripping it — a node that fails to start on a config value is a worse discovery mechanism than one sentence here. Suggest appending "Valid range: 1 to 1048576." to the description.
…ocument the accepted range
|
Claude finished @kamilchodola's task in 10m 41s —— View job Review (round 5,
|
| # | Claim | Verdict |
|---|---|---|
| Low 1 | InvalidConfigurationException |
Fixed. InvalidConfigurationException(string, int) implements IExceptionWithExitCode, so Program.cs maps it to a deliberate exit code rather than a generic unhandled-error path, and it matches the newer validation in this assembly. Checked the using churn compiles clean: System.IO is now genuinely unused in the file, and ExitCodes lives in Nethermind.Config, so the added using is required. One nit inline — ExitCodes reserves 100…199 for config errors and ForbiddenOptionValue = 107 fits this precisely; GeneralError throws away the reason for carrying a code. |
| Low 2 | accepted range documented | Fixed. "accepted range 1 to 1048576" matches ApplyMemoryHint.cs:35-41 exactly (1 << 20), so the generated config docs and the startup guard now agree. |
Also re-verified in this pass (unchanged, still correct)
- Startup ordering.
ApplyMemoryHintis a root step (BuiltInStepsModule.cs:14) andInitTxTypesAndRlpdeclares[RunnerStepDependencies(typeof(ApplyMemoryHint))], so theMemoryAllowance.InstructionStreamCacheSizewrite really does precede EVM work; theInfolog makes a violation observable. EvmPooledMemorybyte budget._cachedArrayBytesis balanced across the only two mutators (Rent's hit path decrements,Return's cache path increments) and cannot drift —Rentis the sole way an array leaves the cache.Rent'sint cachedArrayCount = _cachedArrayCount - 1makes the loop a no-op at count 0, so thecache!deref is unreachable when the cache is null. No new array shape reachesReturnLarge:(1<<16, 1<<18]buffers are diverted away from the shared pools, never towards them.RentSlow's dirty-buffer contract survives the bigger cached sizes. Growth copies only_lastZeroedSizebytes and then zero-extends totarget = min(memory.Length, roundUp(Size, 4 KiB)) >= Size, so every readable byte is cleared regardless of which rent source produced the buffer — which is exactly what the new[TestCase]set asserts, and the 1 MiB case genuinely exercises the shared-pool arm (RentLarge→SafeArrayPoolat exactlyMaxSharedArrayLength).StackPoolTestsis not order-dependent, which I had to check since it assertsIs.SameAsagainst a process-wide[ThreadStatic]. It holds even when a prior deep-recursion test left the thread cache at all 16 slots, becauseRentdecrements beforeReturnincrements — so the return always finds a free slot. No flake.Refreshed_entry_survives_sustained_churn_in_its_setstays deterministic under Windows QPC's 100 ns tick: a tie would needSet(filler_{i-1})→Get(hot)to span under 100 ns across anAssert.EnterMultipleScope()exit and two assertions.- Metric plumbing: all five files touched by
StripedLongalready importNethermind.Core.Threading, all still useInterlockedfor the_main*words so nousingwent dead, andMetricsControllernever writes along[CounterMetric](SetValueis reached only forIMetricObserver/DetailedMetricOnFlagmembers), so the get-only-property form cannot break registration. GC.Add/RemoveMemoryPressurepairing is still symmetric: the only two thresholded sites areDbOnTheRocks.cs:952/:1054, both keyed on the same constant and the same span, and the other pressure users in the repo (HyperClockCacheWrapper,BloomFilter,PageResidencyTracker) are untouched and independently paired.
Findings
Medium
- The EVM opcode benchmark reports
EXTCODESIZE111.7 ns → 841.1 ns (+653%) and it has not been triaged. I don't think it's real, but I don't think it can be dismissed either. The harness already reran flagged-noisy opcodes and aggregated 3 runs per side, and while the base arm is loose (CV 15.5%) the PR arm is tight — CV 3.1% around 841 ns; a tight measurement 7.5× above base is not the usual shape of a noisy cell. And EXTCODESIZE is the opcode most exposed here: its non-peephole path (EvmInstructions.CodeCopy.cs:308) reachesStaticCodeCache.Get→AssociativeCache.Get, so one opcode now pays both new per-lookup costs —Stopwatch.GetTimestamp()on the cache hit (AssociativeCache.cs:174) andThread.GetCurrentProcessorId()+ bounds-checked array add inStripedLong(Metrics.cs:64, viaCacheCodeInfoRepository.cs:41). Neither should cost ~730 ns on a TSC host — but that's the point: two cheap checks discriminate. Re-run the opcode diff, and if it reproduces, read/sys/devices/system/clocksource/clocksource0/current_clocksourceon thebenchmarkrunner. A non-tscclocksource makes the clock read an MMIO read or syscall per code-cache hit, which would be a real block-processing regression that the (presumably TSC) expb arms cannot see. Full reasoning inline. Fix this →
Low
ExitCodes.GeneralErrorwhereExitCodes.ForbiddenOptionValue(107, in the reserved100…199config-error range) is the fitting code — inline, one-word change.- Still no test pinning the
[1, 1 << 20]guard. TwoAssert.Throwscases onExecutewould do it. I called this optional in round 4 and still would; noting only that it's the one new behaviour in the PR without a test.
Verdict
The diff is ready. Rounds 1–4 closed every substantive concern, and d3596ad closes the last two cosmetic ones; I found nothing new in the code this pass, including on a fresh look at the pooling arithmetic and the new tests' order-independence.
What I would not merge past is finding 1 — not because I believe the regression, but because it is currently unexplained on a path this PR touches twice, and the two things that would explain it are a re-run and one cat on the benchmark runner. If it doesn't reproduce, say so and this is a clean merge; if it does, the round-3 fallback (Interlocked on a line-isolated _ticker) is already sketched. Findings 2 and 3 can ride either way.
Not verified: dotnet build / dotnet test are outside my allowed tools in this environment (both dotnet --version and the clocksource read were denied), so this remains a static review. I hand-checked compile-level plumbing for the round-4 delta — InvalidConfigurationException's two-arg ctor, ExitCodes living in Nethermind.Config (so the added using is needed), System.IO now being unused — but compiled and executed nothing, and I could not reproduce the opcode benchmark.
· branch perf/ethcall-concurrency
| if (!ExecutionMetricsFlag.IsActive) return; | ||
| Interlocked.Increment(ref IsBlockProcessingThread ? ref _mainCodeDbCache.Value : ref _otherCodeDbCache.Value); | ||
| if (IsBlockProcessingThread) Interlocked.Increment(ref _mainCodeDbCache.Value); | ||
| else _otherCodeDbCache.Increment(); |
There was a problem hiding this comment.
Medium — the opcode-benchmark bot reports EXTCODESIZE at 111.7 ns → 841.1 ns (+653%) and it has not been triaged.
I am not claiming this is real, but I do not think it can be waved off as noise either, and this is a performance PR:
- It is the only flagged opcode, and the harness already reruns flagged-noisy opcodes and aggregates 3 runs per side (
evm-opcode-benchmark-diff.yml:90,:185-237). The base arm is loose (CV 15.5%) but the PR arm is tight — CV 3.1% around 841 ns. A tight measurement 7.5× above base is not the shape of a noisy cell; noise usually widens the arm that moves. - EXTCODESIZE is, as far as I can tell, the opcode most exposed to this PR. On the non-peephole path (
EvmInstructions.CodeCopy.cs:308) it callsGetCachedCodeInfo, which reachesCacheCodeInfoRepository.GetOrCacheCodeInfo→StaticCodeCache.Get→AssociativeCache.Get. So a single EXTCODESIZE now pays both newly-added per-lookup costs:Stopwatch.GetTimestamp()on the cache hit (AssociativeCache.cs:174), replacingInterlocked.Increment— the exact tradeoff the comment there now documents, including the non-TSC-clocksource case;_otherCodeDbCache.Increment()here, i.e.Thread.GetCurrentProcessorId()+ an array load + bounds check, replacing a directInterlocked.Incrementon a padded word (CacheCodeInfoRepository.cs:41; only whenExecutionMetricsFlag.IsActive).
Neither of those should cost ~730 ns on a TSC host, which is why I lean towards a harness artifact — but that is a guess, and the two candidates above are cheap to discriminate:
- re-run the opcode diff and see whether EXTCODESIZE reproduces;
- if it does, check
cat /sys/devices/system/clocksource/clocksource0/current_clocksourceon thebenchmarkrunner. A non-tscclocksource turns (1) into an MMIO read or a real syscall on every code-cache hit, which would make this a genuine block-processing regression on any host in the same situation — and the expb arms (all TSC, presumably) could not see it.
Either outcome is fine to merge on; an untriaged 7× on a path the PR touches twice is the part I would not merge past. If it reproduces and (1) is the cause, the fallback the round-3 discussion already identified applies: keep Interlocked.Increment but move _ticker off the _epochAndCount line, which addresses the stated false-sharing mechanism without putting a clock read on the hit path.
| { | ||
| throw new InvalidConfigurationException( | ||
| $"Init.{nameof(IInitConfig.InstructionStreamCacheSize)} must be between 1 and {maxInstructionStreamCacheSize}, got {initConfig.InstructionStreamCacheSize}.", | ||
| ExitCodes.GeneralError); |
There was a problem hiding this comment.
Resolved (round-4 Low 1) — one follow-on nit.
InvalidConfigurationException(string, int) is the right type: it implements IExceptionWithExitCode, so Program.cs maps it to a deliberate exit code instead of a generic unhandled-error path, and it matches the newer validation in this assembly (InitializeNetwork.cs:127, PruningTrieStateFactory.cs:196). The dropped using System.IO; is now genuinely unused in this file and the added using Nethermind.Config; is required for ExitCodes — lint-clean either way.
Nit: ExitCodes reserves 100…199 for config errors and already has a name that fits this exactly — ForbiddenOptionValue = 107 (used elsewhere for out-of-range config values). GeneralError works, but a distinct code is the whole reason to carry one.
| ExitCodes.GeneralError); | |
| ExitCodes.ForbiddenOptionValue); |
1 participant
Changes
VmStateand EVM stack pools — the sharedConcurrentQueues cost a contended CAS per call frame and migrate ~33 KB pinned stack arrays across cores under concurrent RPC load; shared queues remain as overflow.Interlockedticker inAssociativeCache/AssociativeKeyCache(the RMW dirtied the line every lookup reads first); coarse-clock eviction age, write-free steady-state hits; test-before-OrinClockCacheBase.MarkAccessed.StripedLong(per-core 128 B slots) for hot shared metric counters (previously one shared "other" word for all RPC/prewarm threads, updated per trie-node read / DB get / code lookup);GC.Add/RemoveMemoryPressureonly for spans ≥ 16 KB (was: twice per DB read).Init.InstructionStreamCacheSizeconfig, default 4096 (was a hardcoded 1024 — the largest default-memory change in this PR, see Remarks). Under concurrency the hot code set outgrows the cache's set associativity and frames silently fall back to the streamless interpreter (own-time ×7 at 300 rps in symbolized perf profiles).JsonRpc.EthModuleConcurrentInstancesdefault 1× → 2× core count +JsonRpcOverloadRejectionsmetric — the caps fail fast and offered concurrency = rate × latency, so at 1× a node rejects override-carrying eth_calls whenever avg latency crosses ~cores/rate; the shedding was invisible (warnings deliberately suppressed).Evidence
Private 497-call mainnet eth_call corpus, 8-core box, ABBA arms vs pinned master. Response parity 497/497 on every arm of every run.
Fully warmed node, idle machine — state fully cached, ~27 ms/call at 300 rps, concurrency ≈ core count (all values ms):
¹ one spike cell at n=3; the loaded-regime table below (n=6) shows the systematic tail behavior.
Partially warmed node — fresh start, caches still filling (typical after restart/snapshot sync or under cache churn): ~45 ms/call at 300 rps pushes offered concurrency past the core count, so queueing and stalls appear — the regime where the degradation actually hurts (n=6 cells per side):
² ~4% at the old 1×-core default cap; both arms above ran with the cap raised.
InstructionStreamCacheSize1024 → 4096 in isolation (same build, env-only A/B, 300 rps — this is the PR's shipped default; the tables above include it): avg −29.5%, p90 −40.6%, p95 −45.9%, failures 1.76% → 0.35%.Block processing safety (expb fusaka, same-session A/B, 3000 payloads):
Mechanism (host perf + symbolized JIT profiles): IPC at 300 rps drops 2.65 → 2.22 at flat clocks, growth entirely in managed code; GC, CPU frequency, and native RocksDB ruled out. The remaining warm 50→300 median inflation (~+22%) is queueing at full core occupancy (300 rps × 27 ms ≈ 8.3 concurrent on 8 cores) — addressable only by reducing per-call CPU; out of scope here.
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Core.Test 5936 / Evm.Test 5021 / Trie.Test 470 / Db.Test 549 / Config.Test 53 green locally (release). One pre-existing environment-flaky JsonRpc hex-parsing test fails identically with and without these changes. Full-corpus eth_call response parity vs master (497/497) on every benchmark arm; block-processing safety via same-session expb fusaka A/B.
Documentation
Requires documentation update
Config items carry their own docs:
Init.InstructionStreamCacheSize(new) and theJsonRpc.EthModuleConcurrentInstancesdefault change.Requires explanation in Release Notes
RPC nodes serving concurrent eth_call traffic: hard "Too many requests" rejections at the old 1×-core default are gone by default; the instruction-stream cache default grew 1024 → 4096, raising worst-case (not typical) stream retention from 512 MiB to 2 GiB — memory-constrained setups can lower it.
Remarks
Memory accounting for the two default changes: the stream-cache default raises the worst-case retention ceiling from 512 MiB to 2 GiB (size × 512 KiB per-entry cap; typical mainnet streams are far smaller, so realistic retention at 4096 is a few hundred MB), and the concurrency default doubles only lazily-created pool retention. Lower
Init.InstructionStreamCacheSizeon memory-constrained setups. Follow-up campaign (per-call CPU toward reth parity) tracked separately; a rebased per-VM arena for EVM frame memory (supersedes #12382) is staged onv7/arena-portfor it.