Skip to content

[RangeIndex] Replace busy-spin snapshot claim with SemaphoreSlim to avoid CPU spikes#1897

Draft
tiagonapoli wants to merge 15 commits into
mainfrom
tiagonapoli/bftree-snapshot-latency-injection
Draft

[RangeIndex] Replace busy-spin snapshot claim with SemaphoreSlim to avoid CPU spikes#1897
tiagonapoli wants to merge 15 commits into
mainfrom
tiagonapoli/bftree-snapshot-latency-injection

Conversation

@tiagonapoli

@tiagonapoli tiagonapoli commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Problem

The per-tree snapshot claim in TreeEntry serialized concurrent CPR snapshots — OnFlush (flush), SnapshotAllTreesForCheckpoint (checkpoint), and SnapshotForMigration (cluster migration) — using a CAS + busy-spin:

while (!TryClaimSnapshot())   // Interlocked.CompareExchange on an int
    Thread.Yield();

However, in case of pretty big BFTree, and since we may be writing multi-gigabyte files, snapshot may take more time, so any waiter will burn CPU for the entire wait. If migration happens during a checkpoint, or flushes happen with migration/checkpoint, we might burn CPU unnecessarily.

Fix

Replace the CAS + spin with a per-tree SemaphoreSlim so waiters block instead of spinning:

snapshotLock.Wait();
try { /* injected latency hook */ CprSnapshotByPtr(...); }
finally { snapshotLock.Release(); }

…ck, don't spin)

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>
@tiagonapoli tiagonapoli changed the title [RangeIndex] Replace busy-spin snapshot claim with SemaphoreSlim (block, don't spin) [RangeIndex] Replace busy-spin snapshot claim with SemaphoreSlim to avoid CPU spikes Jun 23, 2026
Tiago Napoli and others added 14 commits June 22, 2026 18:44
…ms OS tick on Windows)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant