Skip to content

The fixture's lock contention is writer-driven, not a flat tax #2396

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.

Sign up for GitHub

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

Merged
erikdarlingdata merged 1 commit into from
Aug 21, 2026
Merged

The fixture's lock contention is writer-driven, not a flat tax #2396

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions Lite.Tests/SharedDuckDbFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,23 @@ namespace PerformanceMonitorLite.Tests;
/// concurrently across classes.
///
/// <para><b>What that does NOT buy (#2376).</b> The file separation is real, but it does
/// not make the classes independent at RUNTIME: <c>DuckDbInitializer</c>'s reader/writer
/// lock is <c>private static readonly</c>, so every read and write in the suite queues on
/// ONE process-wide lock no matter which database file it targets. Classes therefore still
/// serialize against each other inside their locked sections — through a lock rather than
/// through a collection, and without a collection's ordering.</para>
/// not make the classes independent at RUNTIME: <c>DuckDbInitializer</c>'s lock is
/// <c>private static readonly</c>, so it is ONE lock for the whole process no matter which
/// database file a call targets.</para>
///
/// <para>Be precise about what that costs, because the obvious reading overstates it. The
/// lock is a <c>ReaderWriterLockSlim</c>, so readers do NOT queue behind each other — any
/// number of <c>AcquireReadLock</c> holders run concurrently across classes, and this suite
/// is overwhelmingly readers (~184 read call sites against ~20 write sites). The

@claude claude Bot Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor accuracy nit, flagging only because this comment's whole point is getting the numbers right: the read count checks out (grep -c AcquireReadLock across the repo lands at ~186 call sites, close to the stated ~184), but the write count looks understated. Counting real AcquireWriteLock() invocations (excluding the method definition and the three string-literal references in QueryStoreSliceRepairTests.cs that just search for the pattern) comes out closer to ~26-27, not ~20 — about 30% higher than stated.

This doesn't change the conclusion (still overwhelmingly reader-driven, roughly 7:1 either way), so it's not blocking, but worth a quick double-check of how the write count was derived before merging, given #2376's history of an uncomputed stat.

/// serialization is writer-driven: a writer excludes everyone, and readers block only while
/// a writer holds the lock or is waiting for it. The contention is therefore bursty around
/// the write sites — archival, compaction, CHECKPOINT, the mute and alert-history stores —
/// rather than a flat tax on every database call.</para>
///
/// <para>That distinction decides what a fix could even look like: a collection fixture
/// would serialize the READERS as well, so it does not address writer-driven contention and
/// would cost more than it saves. Nobody has measured how much of the suite's ~190-220s is
/// this, and the honest answer is that it may be very little.</para>
///
/// <para>That lock is correct and must not be narrowed to fix this: production creates
/// several <c>DuckDbInitializer</c> instances over the same <c>App.DatabasePath</c>
Expand Down
Loading