From d10e10633f69bb64253f8c3402133777c6a49af7 Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 15:57:31 -0700 Subject: [PATCH 01/15] [RangeIndex] Replace busy-spin snapshot claim with SemaphoreSlim (block, don't spin) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-tree snapshot claim in TreeEntry serialized concurrent CPR snapshots (OnFlush / checkpoint / migration) with a CAS + Thread.Yield() busy-spin. Because a CPR snapshot is multi-second (bftree's snapshot() sleeps ~1s per phase), any waiter burned a full CPU core for the entire hold. Under contention (e.g. a migration/checkpoint snapshot of a hot index while flushes land on the same tree) this spikes CPU to N cores for N waiters. Replace the CAS + spin with a per-tree SemaphoreSlim so waiters block instead of spinning. A SemaphoreSlim (vs lock/Monitor) keeps an async snapshot path (WaitAsync) open for the future. It is intentionally not disposed: TreeEntry has no disposal lifecycle and the semaphore holds no unmanaged handle (AvailableWaitHandle is never used), mirroring CollectionItemObserver.ResultFoundSemaphore — this also avoids a dispose-vs-in-flight-snapshot race against the lock-free checkpoint path. Adds a DEBUG-only fault injection (RangeIndex_Snapshot_Inject_Latency + WaitOnClearBlocking) and a standalone test that holds the claim with injected latency while 10 concurrent snapshots contend, asserting they block (don't spin) and all complete after release. Measured (10 waiters, 500ms hold window, net10.0 Debug): busy-spin (old): ~10 cores (cpu ~5.1s / 0.5s window) SemaphoreSlim: ~0.03 cores (cpu ~16ms) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Testing/ExceptionInjectionHelper.cs | 13 ++ libs/common/Testing/ExceptionInjectionType.cs | 6 + .../Resp/RangeIndex/RangeIndexManager.cs | 72 +++++---- .../RespRangeIndexTests.cs | 140 ++++++++++++++++++ 4 files changed, 204 insertions(+), 27 deletions(-) diff --git a/libs/common/Testing/ExceptionInjectionHelper.cs b/libs/common/Testing/ExceptionInjectionHelper.cs index 0d215bfb035..2019ef0b756 100644 --- a/libs/common/Testing/ExceptionInjectionHelper.cs +++ b/libs/common/Testing/ExceptionInjectionHelper.cs @@ -149,6 +149,19 @@ public static void WaitOnClear(ExceptionInjectionType exceptionType) Thread.Yield(); } + /// + /// Block (without busy-spinning) while the given injection is enabled, returning once it + /// is disabled. Unlike this consumes negligible CPU while + /// waiting (1 ms polling sleeps), so any significant CPU observed during the wait is + /// attributable to other (busy-spinning) threads. + /// + /// + public static void WaitOnClearBlocking(ExceptionInjectionType exceptionType) + { + while (IsEnabled(exceptionType)) + Thread.Sleep(1); + } + /// /// Wait on clear condition /// diff --git a/libs/common/Testing/ExceptionInjectionType.cs b/libs/common/Testing/ExceptionInjectionType.cs index fbc998efeb2..6ed387c6413 100644 --- a/libs/common/Testing/ExceptionInjectionType.cs +++ b/libs/common/Testing/ExceptionInjectionType.cs @@ -112,5 +112,11 @@ public enum ExceptionInjectionType /// in-flight ProcessRecord. /// RangeIndex_Migration_Receive_Pause_In_ProcessRecord, + /// + /// RangeIndex CPR snapshot: while enabled, the snapshot holds the per-tree snapshot claim + /// (blocking, no CPU) to emulate a slow cpr_snapshot. A concurrent snapshot on the + /// same tree (e.g. migration) busy-spins on the claim, reproducing the high-CPU spin. + /// + RangeIndex_Snapshot_Inject_Latency, } } \ No newline at end of file diff --git a/libs/server/Resp/RangeIndex/RangeIndexManager.cs b/libs/server/Resp/RangeIndex/RangeIndexManager.cs index 2df78025e12..63566c87ed3 100644 --- a/libs/server/Resp/RangeIndex/RangeIndexManager.cs +++ b/libs/server/Resp/RangeIndex/RangeIndexManager.cs @@ -66,6 +66,14 @@ public sealed partial class RangeIndexManager : IDisposable /// Gets the number of live (registered) BfTree indexes (activated + pending). internal int LiveIndexCount => liveIndexes.Count; + /// + /// Test-only: returns the live for the given key, or null + /// if no live (activated) tree exists for it. Used by tests that need to drive the + /// per-tree snapshot claim directly. + /// + internal TreeEntry GetLiveTreeEntryForTest(ReadOnlySpan keyBytes) + => liveIndexes.TryGetValue(KeyId(keyBytes), out var entry) && entry?.Tree != null ? entry : null; + /// Gets the log-tied root directory for RI files. Used by the cluster replication layer /// to determine the target directory for received flush files on the replica side. public string RiLogRoot => riLogRoot; @@ -149,45 +157,55 @@ internal sealed class TreeEntry public int SnapshotPending; /// - /// Per-tree snapshot serialization atomic. 0 = idle; 1 = a snapshot is in flight. - /// Both and - /// claim this before calling - /// cpr_snapshot so the two callers do not race for bftree's internal - /// snapshot_in_progress flag (which would no-op one of them silently). + /// Per-tree snapshot serialization lock (binary semaphore). Held while a CPR snapshot + /// of this tree is in flight, so concurrent snapshot producers — , + /// , and migration — do not race for bftree's + /// internal snapshot_in_progress flag (which would silently no-op one of them). + /// + /// A semaphore (not a spin/CAS) because the critical section is a full CPR + /// snapshot, which is multi-second — waiters must block, not busy-spin, to avoid + /// burning a core each. A SemaphoreSlim (rather than lock/Monitor) keeps + /// the door open to an async snapshot path (WaitAsync) in the future. + /// + /// Intentionally not disposed: has no disposal lifecycle, + /// and the semaphore holds no unmanaged handle (its AvailableWaitHandle is never + /// used), so the GC reclaims it. This also avoids a dispose-vs-in-flight-snapshot race. + /// Mirrors CollectionItemObserver.ResultFoundSemaphore. /// - public int SnapshotInProgress; + private readonly SemaphoreSlim snapshotLock = new(1, 1); - /// Try to claim the per-tree snapshot atomic. Returns true if claimed. - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public bool TryClaimSnapshot() - => Interlocked.CompareExchange(ref SnapshotInProgress, 1, 0) == 0; - - /// Release the per-tree snapshot atomic. - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public void ReleaseSnapshot() - => Volatile.Write(ref SnapshotInProgress, 0); + /// + /// Test-only: true while a snapshot holds . Lets a test + /// detect that the holder has entered the critical section. + /// + internal bool IsSnapshotInProgressForTest => snapshotLock.CurrentCount == 0; /// - /// Spin to claim the per-tree snapshot atomic, take a CPR snapshot of the live tree - /// directly into , then release the claim. The - /// claim serializes against concurrent flush / checkpoint / migration snapshots on - /// the same tree — bftree silently no-ops a cpr_snapshot that races its - /// internal snapshot_in_progress flag, so each caller must hold the claim - /// while it snapshots. bftree takes the destination path as a cpr_snapshot - /// argument and writes a fresh self-contained file, so we snapshot straight to + /// Acquire the per-tree snapshot lock (blocking — no busy-spin), take a CPR snapshot of + /// the live tree directly into , then release the + /// lock. The lock serializes against concurrent flush / checkpoint / migration snapshots + /// on the same tree — bftree silently no-ops a cpr_snapshot that races its + /// internal snapshot_in_progress flag, so each caller must hold the lock while it + /// snapshots. bftree takes the destination path as a cpr_snapshot argument and + /// writes a fresh self-contained file, so we snapshot straight to /// . /// public void SnapshotUnderClaim(string destinationPath) { - while (!TryClaimSnapshot()) - Thread.Yield(); + snapshotLock.Wait(); try { + // Test-only latency injection: while enabled, hold the per-tree snapshot lock + // (blocking, negligible CPU) to emulate a slow cpr_snapshot. Concurrent + // SnapshotUnderClaim callers on the same tree (e.g. migration) block on the + // semaphore above rather than busy-spinning. No-op in Release and whenever the + // injection is disabled. + ExceptionInjectionHelper.WaitOnClearBlocking(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency); BfTreeService.CprSnapshotByPtr(Tree.NativePtr, destinationPath); } finally { - ReleaseSnapshot(); + snapshotLock.Release(); } } @@ -648,7 +666,7 @@ internal void LogOnFlushInvariantViolation(string hashPrefix, long logicalAddres /// /// Live case (stub.TreeHandle != 0): take a CPR snapshot via the /// native handle. CPR is concurrent-safe with workers (no per-key X-lock needed). - /// Per-tree atomic serializes against + /// Per-tree lock serializes against /// concurrent for the same tree (otherwise /// bftree's internal snapshot_in_progress would no-op one of them). /// @@ -784,7 +802,7 @@ internal void ClearCheckpointBarrier() /// to snapshot from; data.bftree was pre-staged by PreStage). /// /// - /// Uses the per-tree atomic to serialize + /// Uses the per-tree lock to serialize /// against concurrent for the same tree. Per-key /// X-lock is NOT taken here — that lock would deadlock if any deferred OnFlush fired /// on the checkpoint thread while it held S-locks on hot-path readers' shards. diff --git a/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs b/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs index da03a66d6af..bda4e8f03c3 100644 --- a/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs +++ b/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs @@ -6,6 +6,10 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; +#if DEBUG +using System.Diagnostics; +using Garnet.common; +#endif using Garnet.server; using NUnit.Framework; using NUnit.Framework.Legacy; @@ -2491,5 +2495,141 @@ public void RangeIndexManagerKeyExistsTest() ClassicAssert.IsTrue(rangeIndexManager.KeyExists(System.Text.Encoding.ASCII.GetBytes("str-key"), ref ctx)); ClassicAssert.IsFalse(rangeIndexManager.KeyExists(System.Text.Encoding.ASCII.GetBytes("nope"), ref ctx)); } + +#if DEBUG + /// + /// Verifies that concurrent CPR snapshots of the same tree serialize by blocking + /// (not busy-spinning). When one snapshot is slow (here, artificial latency injected while + /// holding the per-tree snapshot lock), concurrent snapshots on the same tree — exactly + /// what the migration snapshot path does (SnapshotForMigration → + /// TreeEntry.SnapshotUnderClaim) — park on the SemaphoreSlim rather than + /// burning a core each. + /// + /// The assertions are deterministic: no waiter can complete while the holder holds + /// the lock, and all complete once the injected latency is cleared. The CPU consumed during + /// the wait window is measured and logged only (not asserted) to avoid CI flakiness — with + /// the semaphore it should read ~0 cores; with the previous Thread.Yield() spin it + /// read ~waiterCount cores. + /// + [Test] + public void RIConcurrentSnapshotsBlockWithoutBusySpinTest() + { + const int waiterCount = 10; + + using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); + var db = redis.GetDatabase(0); + + // DISK-backed RI so it mirrors migration (which requires a disk-backed tree) and + // SnapshotUnderClaim writes a real snapshot file to riLogRoot. + db.Execute("RI.CREATE", "snapidx", "DISK", "CACHESIZE", "65536", "MINRECORD", "8"); + db.Execute("RI.SET", "snapidx", "field1", "value1"); + + var rangeIndexManager = server.Provider.StoreWrapper.rangeIndexManager; + var entry = rangeIndexManager.GetLiveTreeEntryForTest("snapidx"u8); + ClassicAssert.IsNotNull(entry, "expected a live BfTree for 'snapidx'"); + + var logRoot = rangeIndexManager.RiLogRoot; + var holderPath = Path.Combine(logRoot, "holder.snapshot.bftree"); + + Exception holderEx = null; + var waiterEx = new Exception[waiterCount]; + var waitersDone = new CountdownEvent(waiterCount); + var waitersCompleted = 0; + + // Arm the latency injection BEFORE the holder acquires, so the holder blocks (negligible + // CPU) while holding the per-tree snapshot lock. + ExceptionInjectionHelper.EnableException(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency); + + var holder = new Thread(() => + { + try { entry.SnapshotUnderClaim(holderPath); } + catch (Exception ex) { holderEx = ex; } + }) + { IsBackground = true, Name = "ri-snapshot-holder" }; + + // Multiple waiters — each emulating the migration snapshot path on the SAME tree. All + // block on the per-tree SemaphoreSlim because the holder owns the lock (no busy-spin). + var waiters = new Thread[waiterCount]; + for (var i = 0; i < waiterCount; i++) + { + var idx = i; + var waiterPath = Path.Combine(logRoot, $"waiter{idx}.snapshot.bftree"); + waiters[idx] = new Thread(() => + { + try { entry.SnapshotUnderClaim(waiterPath); } + catch (Exception ex) { waiterEx[idx] = ex; } + finally + { + Interlocked.Increment(ref waitersCompleted); + waitersDone.Signal(); + } + }) + { IsBackground = true, Name = $"ri-snapshot-waiter-{idx}" }; + } + + try + { + holder.Start(); + + // Wait until the holder has actually acquired the snapshot lock and is parked in + // the injected latency wait. + var deadline = Stopwatch.GetTimestamp(); + while (!entry.IsSnapshotInProgressForTest + && Stopwatch.GetElapsedTime(deadline) < TimeSpan.FromSeconds(10)) + Thread.Yield(); + ClassicAssert.IsTrue(entry.IsSnapshotInProgressForTest, + "holder failed to acquire the snapshot lock"); + + // Start all waiters. + var cpuBefore = Process.GetCurrentProcess().TotalProcessorTime; + var wallBefore = Stopwatch.GetTimestamp(); + foreach (var w in waiters) + w.Start(); + + // Deterministic assertion #1: while the holder holds the lock, NO waiter can + // complete (they are all blocked). The 500ms window is enough to observe in + // Task Manager / dotnet-counters that the waiters consume ~no CPU (unlike the + // previous Thread.Yield() spin, which burned a core each). + var allCompletedWhileBlocked = waitersDone.Wait(TimeSpan.FromMilliseconds(500)); + + var cpuDuringWait = Process.GetCurrentProcess().TotalProcessorTime - cpuBefore; + var wallDuringWait = Stopwatch.GetElapsedTime(wallBefore); + + ClassicAssert.IsFalse(allCompletedWhileBlocked, + "waiters unexpectedly completed while the holder held the snapshot lock"); + ClassicAssert.AreEqual(0, Volatile.Read(ref waitersCompleted), + "no waiter should complete while the holder holds the lock"); + + // Log (do NOT assert) the CPU consumed during the wait window. With the SemaphoreSlim + // this should read ~0 cores; with the previous Thread.Yield() spin it read + // ~waiterCount cores. + TestContext.Progress.WriteLine( + $"[snapshot-wait] waiters={waiterCount} window={wallDuringWait.TotalMilliseconds:F0}ms " + + $"cpu={cpuDuringWait.TotalMilliseconds:F0}ms " + + $"(~{cpuDuringWait.TotalMilliseconds / Math.Max(1, wallDuringWait.TotalMilliseconds):F2} cores)"); + + // Release the injected latency: the holder finishes its real snapshot and releases + // the lock; the waiters then acquire one-by-one and complete. + ExceptionInjectionHelper.DisableException(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency); + + // Deterministic assertion #2: all waiters complete once the lock is released. + // Each does a real (~3s) CPR snapshot serially, so allow a generous timeout. + ClassicAssert.IsTrue(waitersDone.Wait(TimeSpan.FromSeconds(60)), + "waiters did not all complete after the snapshot lock was released"); + } + finally + { + // Always clear the injection so a mid-test failure cannot wedge other tests. + ExceptionInjectionHelper.DisableException(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency); + holder.Join(TimeSpan.FromSeconds(60)); + foreach (var w in waiters) + w.Join(TimeSpan.FromSeconds(60)); + } + + ClassicAssert.IsNull(holderEx, $"holder threw: {holderEx}"); + for (var i = 0; i < waiterCount; i++) + ClassicAssert.IsNull(waiterEx[i], $"waiter {i} threw: {waiterEx[i]}"); + } +#endif } } \ No newline at end of file From 5f3952e1ac841701269bb658e7c2bbffb0aa75c4 Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 18:44:25 -0700 Subject: [PATCH 02/15] Fix inaccurate WaitOnClearBlocking doc (Thread.Sleep(1) rounds to ~15ms OS tick on Windows) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- libs/common/Testing/ExceptionInjectionHelper.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libs/common/Testing/ExceptionInjectionHelper.cs b/libs/common/Testing/ExceptionInjectionHelper.cs index 2019ef0b756..95f3d3bdb15 100644 --- a/libs/common/Testing/ExceptionInjectionHelper.cs +++ b/libs/common/Testing/ExceptionInjectionHelper.cs @@ -150,10 +150,10 @@ public static void WaitOnClear(ExceptionInjectionType exceptionType) } /// - /// Block (without busy-spinning) while the given injection is enabled, returning once it - /// is disabled. Unlike this consumes negligible CPU while - /// waiting (1 ms polling sleeps), so any significant CPU observed during the wait is - /// attributable to other (busy-spinning) threads. + /// Blocks (not busy-spinning) until is disabled, + /// consuming negligible CPU. Test-only: lets a holder park so any CPU observed during the + /// wait is attributable to other (busy-spinning) threads. Polls with short sleeps, so the + /// disable is noticed within roughly one OS timer tick (~15 ms on Windows). /// /// public static void WaitOnClearBlocking(ExceptionInjectionType exceptionType) From d81db4e547475aa37617960c55aa6d35f9f40fb3 Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 18:45:32 -0700 Subject: [PATCH 03/15] Drop stale busy-spin wording from RangeIndex_Snapshot_Inject_Latency doc Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- libs/common/Testing/ExceptionInjectionType.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libs/common/Testing/ExceptionInjectionType.cs b/libs/common/Testing/ExceptionInjectionType.cs index 6ed387c6413..4dde85b290b 100644 --- a/libs/common/Testing/ExceptionInjectionType.cs +++ b/libs/common/Testing/ExceptionInjectionType.cs @@ -113,9 +113,9 @@ public enum ExceptionInjectionType /// RangeIndex_Migration_Receive_Pause_In_ProcessRecord, /// - /// RangeIndex CPR snapshot: while enabled, the snapshot holds the per-tree snapshot claim - /// (blocking, no CPU) to emulate a slow cpr_snapshot. A concurrent snapshot on the - /// same tree (e.g. migration) busy-spins on the claim, reproducing the high-CPU spin. + /// RangeIndex CPR snapshot: while enabled, the snapshot holds the per-tree snapshot lock + /// (blocking, no CPU) to emulate a slow cpr_snapshot, so a test can observe how + /// concurrent snapshots on the same tree (e.g. migration) behave under contention. /// RangeIndex_Snapshot_Inject_Latency, } From 5111cfb74dbbebf716e8a37300cf8c4945576ea6 Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 18:47:20 -0700 Subject: [PATCH 04/15] Condense snapshotLock doc comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Resp/RangeIndex/RangeIndexManager.cs | 22 +++++++------------ 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/libs/server/Resp/RangeIndex/RangeIndexManager.cs b/libs/server/Resp/RangeIndex/RangeIndexManager.cs index 63566c87ed3..a86855c8dcc 100644 --- a/libs/server/Resp/RangeIndex/RangeIndexManager.cs +++ b/libs/server/Resp/RangeIndex/RangeIndexManager.cs @@ -157,20 +157,14 @@ internal sealed class TreeEntry public int SnapshotPending; /// - /// Per-tree snapshot serialization lock (binary semaphore). Held while a CPR snapshot - /// of this tree is in flight, so concurrent snapshot producers — , - /// , and migration — do not race for bftree's - /// internal snapshot_in_progress flag (which would silently no-op one of them). - /// - /// A semaphore (not a spin/CAS) because the critical section is a full CPR - /// snapshot, which is multi-second — waiters must block, not busy-spin, to avoid - /// burning a core each. A SemaphoreSlim (rather than lock/Monitor) keeps - /// the door open to an async snapshot path (WaitAsync) in the future. - /// - /// Intentionally not disposed: has no disposal lifecycle, - /// and the semaphore holds no unmanaged handle (its AvailableWaitHandle is never - /// used), so the GC reclaims it. This also avoids a dispose-vs-in-flight-snapshot race. - /// Mirrors CollectionItemObserver.ResultFoundSemaphore. + /// Per-tree snapshot serialization lock. Held while a CPR snapshot of this tree is in + /// flight so concurrent producers (, + /// , migration) don't race bftree's internal + /// snapshot_in_progress flag (which would silently no-op one). A blocking lock, + /// not a spin, because a CPR snapshot is multi-second; SemaphoreSlim (over + /// lock) leaves an async (WaitAsync) path open. Intentionally not disposed + /// (no unmanaged handle since AvailableWaitHandle is unused), mirroring + /// CollectionItemObserver.ResultFoundSemaphore. /// private readonly SemaphoreSlim snapshotLock = new(1, 1); From 2bb0c040009a7ca38861d027830483a3aa22a04f Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 18:48:55 -0700 Subject: [PATCH 05/15] Condense SnapshotUnderClaim doc comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- libs/server/Resp/RangeIndex/RangeIndexManager.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/libs/server/Resp/RangeIndex/RangeIndexManager.cs b/libs/server/Resp/RangeIndex/RangeIndexManager.cs index a86855c8dcc..0e08d18a06f 100644 --- a/libs/server/Resp/RangeIndex/RangeIndexManager.cs +++ b/libs/server/Resp/RangeIndex/RangeIndexManager.cs @@ -175,14 +175,11 @@ internal sealed class TreeEntry internal bool IsSnapshotInProgressForTest => snapshotLock.CurrentCount == 0; /// - /// Acquire the per-tree snapshot lock (blocking — no busy-spin), take a CPR snapshot of - /// the live tree directly into , then release the - /// lock. The lock serializes against concurrent flush / checkpoint / migration snapshots - /// on the same tree — bftree silently no-ops a cpr_snapshot that races its - /// internal snapshot_in_progress flag, so each caller must hold the lock while it - /// snapshots. bftree takes the destination path as a cpr_snapshot argument and - /// writes a fresh self-contained file, so we snapshot straight to - /// . + /// Under the per-tree snapshot lock (blocking, no busy-spin), take a CPR snapshot of the + /// live tree straight into . The lock serializes + /// concurrent flush / checkpoint / migration snapshots so they don't race bftree's + /// internal snapshot_in_progress flag (which would silently no-op one). bftree + /// writes a fresh self-contained file at the given path. /// public void SnapshotUnderClaim(string destinationPath) { From c93d417528245f7912637b63b3612763530be14e Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 18:49:37 -0700 Subject: [PATCH 06/15] Remove inline injection comment in SnapshotUnderClaim Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- libs/server/Resp/RangeIndex/RangeIndexManager.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/libs/server/Resp/RangeIndex/RangeIndexManager.cs b/libs/server/Resp/RangeIndex/RangeIndexManager.cs index 0e08d18a06f..fb1bb0ab13f 100644 --- a/libs/server/Resp/RangeIndex/RangeIndexManager.cs +++ b/libs/server/Resp/RangeIndex/RangeIndexManager.cs @@ -186,11 +186,6 @@ public void SnapshotUnderClaim(string destinationPath) snapshotLock.Wait(); try { - // Test-only latency injection: while enabled, hold the per-tree snapshot lock - // (blocking, negligible CPU) to emulate a slow cpr_snapshot. Concurrent - // SnapshotUnderClaim callers on the same tree (e.g. migration) block on the - // semaphore above rather than busy-spinning. No-op in Release and whenever the - // injection is disabled. ExceptionInjectionHelper.WaitOnClearBlocking(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency); BfTreeService.CprSnapshotByPtr(Tree.NativePtr, destinationPath); } From 17d9ccdded96a65f27fceb10da5ab1901aa2b2bd Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 18:57:45 -0700 Subject: [PATCH 07/15] Simplify and rename concurrent-snapshot test (RIConcurrentSnapshotsTest) Use Task.Run instead of manual Thread management so exceptions propagate via Task.WaitAll; drop the parallel exception arrays, CountdownEvent, and redundant completion counter. ~120 -> ~55 LOC, same deterministic assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../RespRangeIndexTests.cs | 137 +++++------------- 1 file changed, 35 insertions(+), 102 deletions(-) diff --git a/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs b/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs index bda4e8f03c3..cb0085db242 100644 --- a/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs +++ b/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs @@ -2498,137 +2498,70 @@ public void RangeIndexManagerKeyExistsTest() #if DEBUG /// - /// Verifies that concurrent CPR snapshots of the same tree serialize by blocking - /// (not busy-spinning). When one snapshot is slow (here, artificial latency injected while - /// holding the per-tree snapshot lock), concurrent snapshots on the same tree — exactly - /// what the migration snapshot path does (SnapshotForMigration → - /// TreeEntry.SnapshotUnderClaim) — park on the SemaphoreSlim rather than - /// burning a core each. + /// Concurrent CPR snapshots of the same tree serialize by blocking on the per-tree + /// SemaphoreSlim, not busy-spinning. A holder takes the snapshot lock with injected + /// latency; concurrent snapshots (the migration path: SnapshotForMigration → + /// TreeEntry.SnapshotUnderClaim) park instead of burning a core each. /// - /// The assertions are deterministic: no waiter can complete while the holder holds - /// the lock, and all complete once the injected latency is cleared. The CPU consumed during - /// the wait window is measured and logged only (not asserted) to avoid CI flakiness — with - /// the semaphore it should read ~0 cores; with the previous Thread.Yield() spin it - /// read ~waiterCount cores. + /// Deterministic: no waiter completes while the holder holds the lock; all complete + /// once latency clears. CPU during the wait is logged only (not asserted) to avoid CI + /// flakiness — ~0 cores with the semaphore vs ~waiterCount with the old Thread.Yield() + /// spin. /// [Test] - public void RIConcurrentSnapshotsBlockWithoutBusySpinTest() + public void RIConcurrentSnapshotsTest() { const int waiterCount = 10; using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); var db = redis.GetDatabase(0); - // DISK-backed RI so it mirrors migration (which requires a disk-backed tree) and - // SnapshotUnderClaim writes a real snapshot file to riLogRoot. + // DISK-backed RI mirrors migration and writes a real snapshot file to riLogRoot. db.Execute("RI.CREATE", "snapidx", "DISK", "CACHESIZE", "65536", "MINRECORD", "8"); db.Execute("RI.SET", "snapidx", "field1", "value1"); - var rangeIndexManager = server.Provider.StoreWrapper.rangeIndexManager; - var entry = rangeIndexManager.GetLiveTreeEntryForTest("snapidx"u8); + var manager = server.Provider.StoreWrapper.rangeIndexManager; + var entry = manager.GetLiveTreeEntryForTest("snapidx"u8); ClassicAssert.IsNotNull(entry, "expected a live BfTree for 'snapidx'"); - var logRoot = rangeIndexManager.RiLogRoot; - var holderPath = Path.Combine(logRoot, "holder.snapshot.bftree"); - - Exception holderEx = null; - var waiterEx = new Exception[waiterCount]; - var waitersDone = new CountdownEvent(waiterCount); - var waitersCompleted = 0; + string SnapPath(string name) => Path.Combine(manager.RiLogRoot, $"{name}.snapshot.bftree"); - // Arm the latency injection BEFORE the holder acquires, so the holder blocks (negligible - // CPU) while holding the per-tree snapshot lock. + // Arm the latency injection so the holder parks (negligible CPU) while holding the lock. ExceptionInjectionHelper.EnableException(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency); - - var holder = new Thread(() => - { - try { entry.SnapshotUnderClaim(holderPath); } - catch (Exception ex) { holderEx = ex; } - }) - { IsBackground = true, Name = "ri-snapshot-holder" }; - - // Multiple waiters — each emulating the migration snapshot path on the SAME tree. All - // block on the per-tree SemaphoreSlim because the holder owns the lock (no busy-spin). - var waiters = new Thread[waiterCount]; - for (var i = 0; i < waiterCount; i++) - { - var idx = i; - var waiterPath = Path.Combine(logRoot, $"waiter{idx}.snapshot.bftree"); - waiters[idx] = new Thread(() => - { - try { entry.SnapshotUnderClaim(waiterPath); } - catch (Exception ex) { waiterEx[idx] = ex; } - finally - { - Interlocked.Increment(ref waitersCompleted); - waitersDone.Signal(); - } - }) - { IsBackground = true, Name = $"ri-snapshot-waiter-{idx}" }; - } - try { - holder.Start(); - - // Wait until the holder has actually acquired the snapshot lock and is parked in - // the injected latency wait. - var deadline = Stopwatch.GetTimestamp(); - while (!entry.IsSnapshotInProgressForTest - && Stopwatch.GetElapsedTime(deadline) < TimeSpan.FromSeconds(10)) - Thread.Yield(); - ClassicAssert.IsTrue(entry.IsSnapshotInProgressForTest, + var holder = Task.Run(() => entry.SnapshotUnderClaim(SnapPath("holder"))); + ClassicAssert.IsTrue( + SpinWait.SpinUntil(() => entry.IsSnapshotInProgressForTest, TimeSpan.FromSeconds(10)), "holder failed to acquire the snapshot lock"); - // Start all waiters. - var cpuBefore = Process.GetCurrentProcess().TotalProcessorTime; - var wallBefore = Stopwatch.GetTimestamp(); - foreach (var w in waiters) - w.Start(); - - // Deterministic assertion #1: while the holder holds the lock, NO waiter can - // complete (they are all blocked). The 500ms window is enough to observe in - // Task Manager / dotnet-counters that the waiters consume ~no CPU (unlike the - // previous Thread.Yield() spin, which burned a core each). - var allCompletedWhileBlocked = waitersDone.Wait(TimeSpan.FromMilliseconds(500)); - - var cpuDuringWait = Process.GetCurrentProcess().TotalProcessorTime - cpuBefore; - var wallDuringWait = Stopwatch.GetElapsedTime(wallBefore); - - ClassicAssert.IsFalse(allCompletedWhileBlocked, - "waiters unexpectedly completed while the holder held the snapshot lock"); - ClassicAssert.AreEqual(0, Volatile.Read(ref waitersCompleted), - "no waiter should complete while the holder holds the lock"); - - // Log (do NOT assert) the CPU consumed during the wait window. With the SemaphoreSlim - // this should read ~0 cores; with the previous Thread.Yield() spin it read - // ~waiterCount cores. + // Waiters contend on the SAME tree; they block on the semaphore (no busy-spin). + var cpu0 = Process.GetCurrentProcess().TotalProcessorTime; + var sw = Stopwatch.StartNew(); + var waiters = Enumerable.Range(0, waiterCount) + .Select(i => Task.Run(() => entry.SnapshotUnderClaim(SnapPath($"waiter{i}")))) + .ToArray(); + + // #1: while the holder holds the lock, no waiter can complete. + Thread.Sleep(500); + var cpuMs = (Process.GetCurrentProcess().TotalProcessorTime - cpu0).TotalMilliseconds; + var wallMs = sw.Elapsed.TotalMilliseconds; + ClassicAssert.IsFalse(waiters.Any(t => t.IsCompletedSuccessfully), + "a waiter completed while the holder held the snapshot lock"); TestContext.Progress.WriteLine( - $"[snapshot-wait] waiters={waiterCount} window={wallDuringWait.TotalMilliseconds:F0}ms " + - $"cpu={cpuDuringWait.TotalMilliseconds:F0}ms " + - $"(~{cpuDuringWait.TotalMilliseconds / Math.Max(1, wallDuringWait.TotalMilliseconds):F2} cores)"); + $"[snapshot-wait] waiters={waiterCount} window={wallMs:F0}ms cpu={cpuMs:F0}ms (~{cpuMs / Math.Max(1, wallMs):F2} cores)"); - // Release the injected latency: the holder finishes its real snapshot and releases - // the lock; the waiters then acquire one-by-one and complete. + // Release the latency; holder + waiters each run a real (~3s) snapshot serially. ExceptionInjectionHelper.DisableException(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency); - // Deterministic assertion #2: all waiters complete once the lock is released. - // Each does a real (~3s) CPR snapshot serially, so allow a generous timeout. - ClassicAssert.IsTrue(waitersDone.Wait(TimeSpan.FromSeconds(60)), - "waiters did not all complete after the snapshot lock was released"); + // #2: everything completes (and any fault surfaces) once the lock is released. + ClassicAssert.IsTrue(Task.WaitAll(waiters.Append(holder).ToArray(), TimeSpan.FromSeconds(90)), + "snapshots did not all complete after the lock was released"); } finally { - // Always clear the injection so a mid-test failure cannot wedge other tests. ExceptionInjectionHelper.DisableException(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency); - holder.Join(TimeSpan.FromSeconds(60)); - foreach (var w in waiters) - w.Join(TimeSpan.FromSeconds(60)); } - - ClassicAssert.IsNull(holderEx, $"holder threw: {holderEx}"); - for (var i = 0; i < waiterCount; i++) - ClassicAssert.IsNull(waiterEx[i], $"waiter {i} threw: {waiterEx[i]}"); } #endif } From 69d416e9fbca7865d409ba7df191bfdb697a7322 Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 18:59:49 -0700 Subject: [PATCH 08/15] Rename WaitOnClearBlocking -> WaitOnClearWithThreadSleep; trim doc Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- libs/common/Testing/ExceptionInjectionHelper.cs | 6 ++---- libs/server/Resp/RangeIndex/RangeIndexManager.cs | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/libs/common/Testing/ExceptionInjectionHelper.cs b/libs/common/Testing/ExceptionInjectionHelper.cs index 95f3d3bdb15..e9adcd671c6 100644 --- a/libs/common/Testing/ExceptionInjectionHelper.cs +++ b/libs/common/Testing/ExceptionInjectionHelper.cs @@ -151,12 +151,10 @@ public static void WaitOnClear(ExceptionInjectionType exceptionType) /// /// Blocks (not busy-spinning) until is disabled, - /// consuming negligible CPU. Test-only: lets a holder park so any CPU observed during the - /// wait is attributable to other (busy-spinning) threads. Polls with short sleeps, so the - /// disable is noticed within roughly one OS timer tick (~15 ms on Windows). + /// consuming negligible CPU. /// /// - public static void WaitOnClearBlocking(ExceptionInjectionType exceptionType) + public static void WaitOnClearWithThreadSleep(ExceptionInjectionType exceptionType) { while (IsEnabled(exceptionType)) Thread.Sleep(1); diff --git a/libs/server/Resp/RangeIndex/RangeIndexManager.cs b/libs/server/Resp/RangeIndex/RangeIndexManager.cs index fb1bb0ab13f..95388936ba0 100644 --- a/libs/server/Resp/RangeIndex/RangeIndexManager.cs +++ b/libs/server/Resp/RangeIndex/RangeIndexManager.cs @@ -186,7 +186,7 @@ public void SnapshotUnderClaim(string destinationPath) snapshotLock.Wait(); try { - ExceptionInjectionHelper.WaitOnClearBlocking(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency); + ExceptionInjectionHelper.WaitOnClearWithThreadSleep(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency); BfTreeService.CprSnapshotByPtr(Tree.NativePtr, destinationPath); } finally From 916f40ad9daa473369d6641832abae63df8b44ea Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 19:01:09 -0700 Subject: [PATCH 09/15] Trim WaitOnClearWithThreadSleep doc to one line Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- libs/common/Testing/ExceptionInjectionHelper.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libs/common/Testing/ExceptionInjectionHelper.cs b/libs/common/Testing/ExceptionInjectionHelper.cs index e9adcd671c6..734f9435464 100644 --- a/libs/common/Testing/ExceptionInjectionHelper.cs +++ b/libs/common/Testing/ExceptionInjectionHelper.cs @@ -150,8 +150,7 @@ public static void WaitOnClear(ExceptionInjectionType exceptionType) } /// - /// Blocks (not busy-spinning) until is disabled, - /// consuming negligible CPU. + /// Blocks until is disabled. /// /// public static void WaitOnClearWithThreadSleep(ExceptionInjectionType exceptionType) From e7a119ff372d358c0bad5722f6a78c35a8413949 Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 19:04:48 -0700 Subject: [PATCH 10/15] Trim snapshotLock doc comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- libs/server/Resp/RangeIndex/RangeIndexManager.cs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/libs/server/Resp/RangeIndex/RangeIndexManager.cs b/libs/server/Resp/RangeIndex/RangeIndexManager.cs index 95388936ba0..293191e68e1 100644 --- a/libs/server/Resp/RangeIndex/RangeIndexManager.cs +++ b/libs/server/Resp/RangeIndex/RangeIndexManager.cs @@ -157,14 +157,7 @@ internal sealed class TreeEntry public int SnapshotPending; /// - /// Per-tree snapshot serialization lock. Held while a CPR snapshot of this tree is in - /// flight so concurrent producers (, - /// , migration) don't race bftree's internal - /// snapshot_in_progress flag (which would silently no-op one). A blocking lock, - /// not a spin, because a CPR snapshot is multi-second; SemaphoreSlim (over - /// lock) leaves an async (WaitAsync) path open. Intentionally not disposed - /// (no unmanaged handle since AvailableWaitHandle is unused), mirroring - /// CollectionItemObserver.ResultFoundSemaphore. + /// Serializes CPR snapshots of this tree; concurrent snapshots are not allowed. /// private readonly SemaphoreSlim snapshotLock = new(1, 1); From d7e730c3302a6991188632ff6124f242028a0e8f Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 19:05:18 -0700 Subject: [PATCH 11/15] Drop 'blocking, no busy-spin' from SnapshotUnderClaim doc Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- libs/server/Resp/RangeIndex/RangeIndexManager.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libs/server/Resp/RangeIndex/RangeIndexManager.cs b/libs/server/Resp/RangeIndex/RangeIndexManager.cs index 293191e68e1..d06c75e0c52 100644 --- a/libs/server/Resp/RangeIndex/RangeIndexManager.cs +++ b/libs/server/Resp/RangeIndex/RangeIndexManager.cs @@ -168,11 +168,11 @@ internal sealed class TreeEntry internal bool IsSnapshotInProgressForTest => snapshotLock.CurrentCount == 0; /// - /// Under the per-tree snapshot lock (blocking, no busy-spin), take a CPR snapshot of the - /// live tree straight into . The lock serializes - /// concurrent flush / checkpoint / migration snapshots so they don't race bftree's - /// internal snapshot_in_progress flag (which would silently no-op one). bftree - /// writes a fresh self-contained file at the given path. + /// Under the per-tree snapshot lock, take a CPR snapshot of the live tree straight into + /// . The lock serializes concurrent flush / checkpoint + /// / migration snapshots so they don't race bftree's internal snapshot_in_progress + /// flag (which would silently no-op one). bftree writes a fresh self-contained file at + /// the given path. /// public void SnapshotUnderClaim(string destinationPath) { From 04f835c2519f5a3ef26c59b48d6609fa3729a958 Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 19:06:04 -0700 Subject: [PATCH 12/15] Trim SnapshotUnderClaim doc to just the serialization note Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- libs/server/Resp/RangeIndex/RangeIndexManager.cs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/libs/server/Resp/RangeIndex/RangeIndexManager.cs b/libs/server/Resp/RangeIndex/RangeIndexManager.cs index d06c75e0c52..57cb82f4c1e 100644 --- a/libs/server/Resp/RangeIndex/RangeIndexManager.cs +++ b/libs/server/Resp/RangeIndex/RangeIndexManager.cs @@ -168,11 +168,8 @@ internal sealed class TreeEntry internal bool IsSnapshotInProgressForTest => snapshotLock.CurrentCount == 0; /// - /// Under the per-tree snapshot lock, take a CPR snapshot of the live tree straight into - /// . The lock serializes concurrent flush / checkpoint - /// / migration snapshots so they don't race bftree's internal snapshot_in_progress - /// flag (which would silently no-op one). bftree writes a fresh self-contained file at - /// the given path. + /// Under the per-tree snapshot lock (which serializes concurrent snapshots), take a CPR + /// snapshot of the live tree straight into . /// public void SnapshotUnderClaim(string destinationPath) { From 53fef8b716b9e4571cd238a6bd7086f0986b5175 Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 19:14:39 -0700 Subject: [PATCH 13/15] Collapse multi-line calls to single lines in RIConcurrentSnapshotsTest Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../RespRangeIndexTests.cs | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs b/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs index cb0085db242..0d10cf69ac8 100644 --- a/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs +++ b/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs @@ -2531,32 +2531,25 @@ public void RIConcurrentSnapshotsTest() try { var holder = Task.Run(() => entry.SnapshotUnderClaim(SnapPath("holder"))); - ClassicAssert.IsTrue( - SpinWait.SpinUntil(() => entry.IsSnapshotInProgressForTest, TimeSpan.FromSeconds(10)), - "holder failed to acquire the snapshot lock"); + ClassicAssert.IsTrue(SpinWait.SpinUntil(() => entry.IsSnapshotInProgressForTest, TimeSpan.FromSeconds(10)), "holder failed to acquire the snapshot lock"); // Waiters contend on the SAME tree; they block on the semaphore (no busy-spin). var cpu0 = Process.GetCurrentProcess().TotalProcessorTime; var sw = Stopwatch.StartNew(); - var waiters = Enumerable.Range(0, waiterCount) - .Select(i => Task.Run(() => entry.SnapshotUnderClaim(SnapPath($"waiter{i}")))) - .ToArray(); + var waiters = Enumerable.Range(0, waiterCount).Select(i => Task.Run(() => entry.SnapshotUnderClaim(SnapPath($"waiter{i}")))).ToArray(); // #1: while the holder holds the lock, no waiter can complete. Thread.Sleep(500); var cpuMs = (Process.GetCurrentProcess().TotalProcessorTime - cpu0).TotalMilliseconds; var wallMs = sw.Elapsed.TotalMilliseconds; - ClassicAssert.IsFalse(waiters.Any(t => t.IsCompletedSuccessfully), - "a waiter completed while the holder held the snapshot lock"); - TestContext.Progress.WriteLine( - $"[snapshot-wait] waiters={waiterCount} window={wallMs:F0}ms cpu={cpuMs:F0}ms (~{cpuMs / Math.Max(1, wallMs):F2} cores)"); + ClassicAssert.IsFalse(waiters.Any(t => t.IsCompletedSuccessfully), "a waiter completed while the holder held the snapshot lock"); + TestContext.Progress.WriteLine($"[snapshot-wait] waiters={waiterCount} window={wallMs:F0}ms cpu={cpuMs:F0}ms (~{cpuMs / Math.Max(1, wallMs):F2} cores)"); // Release the latency; holder + waiters each run a real (~3s) snapshot serially. ExceptionInjectionHelper.DisableException(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency); // #2: everything completes (and any fault surfaces) once the lock is released. - ClassicAssert.IsTrue(Task.WaitAll(waiters.Append(holder).ToArray(), TimeSpan.FromSeconds(90)), - "snapshots did not all complete after the lock was released"); + ClassicAssert.IsTrue(Task.WaitAll(waiters.Append(holder).ToArray(), TimeSpan.FromSeconds(90)), "snapshots did not all complete after the lock was released"); } finally { From 79fa589a9ddb3e8997b1c10f8d58b5fdce855df0 Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 19:17:16 -0700 Subject: [PATCH 14/15] Remove CPU measurement from RIConcurrentSnapshotsTest Drop the Process/Stopwatch CPU logging (and the now-unused System.Diagnostics using); the deterministic block/release assertions are the test's purpose. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Garnet.test.rangeindex/RespRangeIndexTests.cs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs b/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs index 0d10cf69ac8..dc3fd9ab5f2 100644 --- a/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs +++ b/test/standalone/Garnet.test.rangeindex/RespRangeIndexTests.cs @@ -7,7 +7,6 @@ using System.Threading; using System.Threading.Tasks; #if DEBUG -using System.Diagnostics; using Garnet.common; #endif using Garnet.server; @@ -2504,9 +2503,7 @@ public void RangeIndexManagerKeyExistsTest() /// TreeEntry.SnapshotUnderClaim) park instead of burning a core each. /// /// Deterministic: no waiter completes while the holder holds the lock; all complete - /// once latency clears. CPU during the wait is logged only (not asserted) to avoid CI - /// flakiness — ~0 cores with the semaphore vs ~waiterCount with the old Thread.Yield() - /// spin. + /// once the latency clears. /// [Test] public void RIConcurrentSnapshotsTest() @@ -2534,16 +2531,11 @@ public void RIConcurrentSnapshotsTest() ClassicAssert.IsTrue(SpinWait.SpinUntil(() => entry.IsSnapshotInProgressForTest, TimeSpan.FromSeconds(10)), "holder failed to acquire the snapshot lock"); // Waiters contend on the SAME tree; they block on the semaphore (no busy-spin). - var cpu0 = Process.GetCurrentProcess().TotalProcessorTime; - var sw = Stopwatch.StartNew(); var waiters = Enumerable.Range(0, waiterCount).Select(i => Task.Run(() => entry.SnapshotUnderClaim(SnapPath($"waiter{i}")))).ToArray(); // #1: while the holder holds the lock, no waiter can complete. Thread.Sleep(500); - var cpuMs = (Process.GetCurrentProcess().TotalProcessorTime - cpu0).TotalMilliseconds; - var wallMs = sw.Elapsed.TotalMilliseconds; ClassicAssert.IsFalse(waiters.Any(t => t.IsCompletedSuccessfully), "a waiter completed while the holder held the snapshot lock"); - TestContext.Progress.WriteLine($"[snapshot-wait] waiters={waiterCount} window={wallMs:F0}ms cpu={cpuMs:F0}ms (~{cpuMs / Math.Max(1, wallMs):F2} cores)"); // Release the latency; holder + waiters each run a real (~3s) snapshot serially. ExceptionInjectionHelper.DisableException(ExceptionInjectionType.RangeIndex_Snapshot_Inject_Latency); From c8f719c36d2ef8813a1f5a40726232d376e2c528 Mon Sep 17 00:00:00 2001 From: Tiago Napoli Date: Mon, 22 Jun 2026 19:24:41 -0700 Subject: [PATCH 15/15] docs(rangeindex): document CPR snapshot performance across tree sizes Add a 'Snapshot performance' subsection with measured snapshot times for tree sizes 32MB-4GB (~3s fixed floor + ~linear with data size) and a key-count variation showing snapshot time is independent of key count at constant data volume. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- website/docs/commands/range-index.md | 35 ++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/website/docs/commands/range-index.md b/website/docs/commands/range-index.md index 2a811f12583..8b96ea96290 100644 --- a/website/docs/commands/range-index.md +++ b/website/docs/commands/range-index.md @@ -44,6 +44,41 @@ Range Index is fully integrated with Garnet's checkpoint and AOF mechanisms: - **Eviction and lazy restore:** When memory pressure causes the stub to be evicted from the log, the BfTree data file is preserved. On next access, the tree is lazily restored from its snapshot. +#### Snapshot performance + +A BfTree checkpoint takes a CPR (Concurrent Prefix Recovery) snapshot, which is synchronous and +non-blocking to concurrent readers/writers. It has a **fixed floor of ~3 seconds** (the snapshot +state machine advances through several phases, each with a short barrier) plus a component that +grows roughly **linearly with the tree's data size**. The snapshot is dominated by total data +**size**, not the number of keys. + +Measured on a single disk-backed tree (local NVMe, 1 KB values, cache large enough to keep the +tree resident; times are approximate and hardware-dependent): + +| Tree data | Keys | Snapshot file | Snapshot time | +|-----------|------|---------------|---------------| +| 32 MB | 32 K | 39 MB | ~3.0 s | +| 64 MB | 65 K | 78 MB | ~3.1 s | +| 128 MB | 129 K| 145 MB | ~3.2 s | +| 512 MB | 516 K| 594 MB | ~4.0 s | +| 1 GB | 1.0 M| 1.16 GB | ~5.1 s | +| 2 GB | 2.1 M| 2.44 GB | ~6.3 s | +| 4 GB | 4.1 M| 4.90 GB | ~8.6 s | + +Holding the data size roughly constant (~500 MB) while varying the value size — and therefore the +key count — shows snapshot time is **independent of key count**: + +| Value size | Keys | Snapshot time | +|------------|-------|---------------| +| 64 B | 6.7 M | ~4.2 s | +| 256 B | 2.0 M | ~4.0 s | +| 1 KB | 516 K | ~4.1 s | +| 4 KB | 131 K | ~4.0 s | + +> **Note:** Each live tree is snapshotted independently during a checkpoint, and snapshots of the +> same tree are serialized. A checkpoint of many large indexes therefore scales with the sum of +> their sizes. + --- ## Lifecycle Commands