-
Notifications
You must be signed in to change notification settings - Fork 720
perf(rpc): fix eth_call degradation under concurrent load (50→300 rps) #12870
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
1c29cfb
3b57e2c
faedf11
ccf2b25
29ff23f
c6f0aa1
94e5925
9999172
d1cc85c
d3596ad
d2e9f94
18d4809
0dbce07
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| // SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited | ||
| // SPDX-License-Identifier: LGPL-3.0-only | ||
|
|
||
| using System.Threading.Tasks; | ||
| using Nethermind.Core.Threading; | ||
| using NUnit.Framework; | ||
|
|
||
| namespace Nethermind.Core.Test.Threading; | ||
|
|
||
| [Parallelizable(ParallelScope.All)] | ||
| public class StripedLongTests | ||
| { | ||
| [Test] | ||
| public void Sum_is_exact_under_concurrent_mixed_adds() | ||
| { | ||
| StripedLong counter = new(); | ||
| const int threads = 8; | ||
| const int iterations = 100_000; | ||
|
|
||
| Parallel.For(0, threads, _ => | ||
| { | ||
| for (int i = 0; i < iterations; i++) | ||
| { | ||
| counter.Increment(); | ||
| counter.Add(3); | ||
| counter.Add(-2); | ||
| } | ||
| }); | ||
|
|
||
| Assert.That(counter.Sum, Is.EqualTo((long)threads * iterations * 2)); | ||
| } | ||
|
|
||
| [Test] | ||
| public void Negative_totals_are_representable() | ||
| { | ||
| StripedLong counter = new(); | ||
| counter.Add(-5); | ||
| counter.Increment(); | ||
| Assert.That(counter.Sum, Is.EqualTo(-4)); | ||
| } | ||
|
|
||
| [Test] | ||
| public void New_counter_sums_to_zero() => Assert.That(new StripedLong().Sum, Is.Zero); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| // SPDX-License-Identifier: LGPL-3.0-only | ||
|
|
||
| using System; | ||
| using System.Diagnostics; | ||
| using System.Numerics; | ||
| using System.Runtime.CompilerServices; | ||
| using System.Runtime.InteropServices; | ||
|
|
@@ -79,13 +80,6 @@ public sealed class AssociativeCache<TKey, TValue> | |
| /// </summary> | ||
| private long _epochAndCount; | ||
|
|
||
| /// <summary> | ||
| /// Monotonic counter for eviction-age tracking. Interlocked.Increment is faster | ||
| /// than Stopwatch.GetTimestamp() (RDTSC) — ~7.6ns vs ~19ns single-threaded, | ||
| /// and scales better under contention (20% faster at 8 threads). | ||
| /// </summary> | ||
| private long _ticker; | ||
|
|
||
| public int Count => ReadCount(ref _epochAndCount); | ||
|
|
||
| public AssociativeCache(int maxCapacity) | ||
|
|
@@ -166,11 +160,18 @@ private bool TryGetCore<TRefreshTicker>(in TKey key, out TValue? value) | |
| if (h1 == h2 && storedKey.Equals(in key)) | ||
| { | ||
| // JIT eliminates this branch entirely per TRefreshTicker instantiation. | ||
| // Eviction age uses the high-resolution clock rather than a shared counter: a | ||
| // per-hit Interlocked on a cache-wide field is a serialized cross-core RMW under | ||
| // concurrent readers (and it dirtied the line _epochAndCount lives on, which | ||
| // every TryGet reads first). Single-threaded the clock read loses a few ns to the | ||
| // old Interlocked (and more on hosts whose clocksource is not TSC), but it writes | ||
| // only this entry's own line, so hits scale with reader count — the regime that | ||
| // motivated the change. Do not flip back to a shared counter for the ns. | ||
| // Ticker store without the set gate is safe: 8-byte aligned long is atomic on | ||
| // 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); | ||
| e.Ticker = Stopwatch.GetTimestamp(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)"):
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 |
||
| value = storedValue; | ||
| return true; | ||
|
Comment on lines
171
to
176
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
That interacts directly with the other half of this PR: Suggestions, in rough order of preference:
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 |
||
| } | ||
|
|
@@ -232,8 +233,7 @@ private bool SetCore(in TKey key, TValue val, int baseIdx, long hashPart) | |
| { | ||
| if ((h & HashMask) == hashPart && e.Key.Equals(in key)) | ||
| { | ||
| long now = Interlocked.Increment(ref _ticker); | ||
| WriteEntry(ref e, h, in key, val, tagToStore, now); | ||
| WriteEntry(ref e, h, in key, val, tagToStore, Stopwatch.GetTimestamp()); | ||
| return false; | ||
| } | ||
| } | ||
|
|
@@ -249,7 +249,7 @@ private bool SetCore(in TKey key, TValue val, int baseIdx, long hashPart) | |
|
|
||
| if (ReadEpoch(ref _epochAndCount) != epochTag) continue; | ||
|
|
||
| long timestamp = Interlocked.Increment(ref _ticker); | ||
| long timestamp = Stopwatch.GetTimestamp(); | ||
| int target = bestEmpty >= 0 | ||
| ? bestEmpty | ||
| : bestStale >= 0 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| // SPDX-License-Identifier: LGPL-3.0-only | ||
|
|
||
| using System; | ||
| using System.Diagnostics; | ||
| using System.Numerics; | ||
| using System.Runtime.CompilerServices; | ||
| using System.Runtime.InteropServices; | ||
|
|
@@ -29,7 +30,6 @@ public sealed class AssociativeKeyCache<TKey> | |
| private readonly int _hashShift; | ||
| private readonly int[] _setGates; | ||
| private long _epochAndCount; | ||
| private long _ticker; | ||
|
|
||
| public int Count => ReadCount(ref _epochAndCount); | ||
|
|
||
|
|
@@ -95,11 +95,18 @@ private bool GetCore<TRefreshTicker>(in TKey key) | |
| if (h1 == h2 && storedKey.Equals(in key)) | ||
| { | ||
| // JIT eliminates this branch entirely per TRefreshTicker instantiation. | ||
| // Eviction age uses the high-resolution clock rather than a shared counter: a | ||
| // per-hit Interlocked on a cache-wide field is a serialized cross-core RMW under | ||
| // concurrent readers (and it dirtied the line _epochAndCount lives on, which | ||
| // every Get reads first). Single-threaded the clock read loses a few ns to the | ||
| // old Interlocked (and more on hosts whose clocksource is not TSC), but it writes | ||
| // only this entry's own line, so hits scale with reader count — the regime that | ||
| // motivated the change. Do not flip back to a shared counter for the ns. | ||
| // Ticker store without the set gate is safe: 8-byte aligned long is atomic on | ||
| // 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); | ||
| e.Ticker = Stopwatch.GetTimestamp(); | ||
| return true; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved (Medium 2) — with one coverage gap.
Gap: Two notes on the test itself, neither blocking:
|
||
| } | ||
| } | ||
|
|
@@ -158,7 +165,7 @@ private bool SetCore(in TKey key, int baseIdx, long hashPart) | |
| // Unlike AssociativeCache.SetCore (which calls WriteEntry to update the value), | ||
| // the key-only variant has nothing to write, so a bare ticker store suffices. | ||
| // The seqlock header is unchanged, which is correct: readers see a stable entry. | ||
| e.Ticker = Interlocked.Increment(ref _ticker); | ||
| e.Ticker = Stopwatch.GetTimestamp(); | ||
| return false; | ||
| } | ||
| } | ||
|
|
@@ -174,7 +181,7 @@ private bool SetCore(in TKey key, int baseIdx, long hashPart) | |
|
|
||
| if (ReadEpoch(ref _epochAndCount) != epochTag) continue; | ||
|
|
||
| long timestamp = Interlocked.Increment(ref _ticker); | ||
| long timestamp = Stopwatch.GetTimestamp(); | ||
| int target = bestEmpty >= 0 | ||
| ? bestEmpty | ||
| : bestStale >= 0 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| // SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited | ||
| // SPDX-License-Identifier: LGPL-3.0-only | ||
|
|
||
| using System; | ||
| using System.Numerics; | ||
| using System.Runtime.CompilerServices; | ||
| using System.Threading; | ||
|
|
||
| namespace Nethermind.Core.Threading; | ||
|
|
||
| /// <summary> | ||
| /// Additive counter safe for hot concurrent paths: increments land on a per-core slot, reads sum | ||
| /// the slots. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// A shared counter word turns every increment into a serialized cross-core cache-line transfer | ||
| /// once several threads hit it (RPC workers, prewarm workers). Striping by | ||
| /// <see cref="Thread.GetCurrentProcessorId"/> keeps the RMW local to the core in the common case; | ||
| /// the atomic add only guards against threads that share or migrate between cores. Slots are | ||
| /// 128-byte spaced — same isolation as <see cref="CacheLinePaddedLong"/> (adjacent-line prefetch | ||
| /// pairs lines) — with a leading pad stride so the first slot is also isolated from whatever | ||
| /// precedes the array in memory. Each instance allocates (stripes + 1) * 128 bytes, where stripes | ||
| /// is <see cref="Environment.ProcessorCount"/> rounded up to a power of two, captured once at | ||
| /// type initialization (later CPU hot-add is not tracked; the mask just folds new ids onto | ||
| /// existing slots). Reads are O(stripes) and torn only across slots: fine for metrics, not for | ||
| /// invariants. | ||
| /// </remarks> | ||
| public sealed class StripedLong | ||
| { | ||
| // 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 + 2) * SlotStride]; | ||
|
|
||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public void Add(long value) | ||
|
Comment on lines
+30
to
+37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Low — a few notes on the new primitive; no correctness problems found.
There's no unit test for this type. A concurrent- |
||
| => Interlocked.Add(ref _slots[((Thread.GetCurrentProcessorId() & s_stripeMask) + 1) * SlotStride], value); | ||
|
|
||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public void Increment() => Add(1); | ||
|
|
||
| public long Sum | ||
| { | ||
| get | ||
| { | ||
| long[] slots = _slots; | ||
| long sum = 0; | ||
| for (int stripe = 0; stripe <= s_stripeMask; stripe++) | ||
| { | ||
| sum += Volatile.Read(ref slots[(stripe + 1) * SlotStride]); | ||
| } | ||
| return sum; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Low — the accepted range is now enforced but not documented.
ApplyMemoryHint.cs:34-38rejects 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 (percoding-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.