Repository navigation
The fixture's lock contention is writer-driven, not a flat tax - #2396
Conversation
#2385 corrected the comment's claim that IClassFixture keeps cross-class parallelism intact, and overshot in the other direction: it now says every read and write in the suite queues on one process-wide lock. That is not what a ReaderWriterLockSlim does. Readers do not queue behind each other, and this suite is overwhelmingly readers -- roughly 184 AcquireReadLock call sites against 20 write sites. Readers only block while a writer holds the lock or is waiting for it, so the cost is bursty around the write sites rather than charged to every database call. The distinction is the whole practical point of the paragraph. Someone reading "every read and write queues" would reasonably reach for a collection fixture, which serializes the readers too and so cannot help writer-driven contention -- it can only make it worse. Says so explicitly, and says plainly that nobody has measured how much of the suite's ~190-220s this actually is. Refs #2376 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// <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 |
There was a problem hiding this comment.
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.
|
Reviewed. This is a doc-comment-only change in The core technical correction is accurate: One minor accuracy nit left as an inline comment: the stated "~184 read call sites against ~20 write sites" undercounts the write side — actual No correctness, security, or performance concerns — this is a comment-only change. |
The 3.5.1 entry credited #2385 with fixing the comment. #2385 replaced one wrong claim with another -- readers do not queue behind each other on a ReaderWriterLockSlim -- and #2396 is what makes it accurate. Worth saying in the entry rather than quietly citing the corrected version, because the distinction is the part that tells a reader a collection fixture would make it worse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Follow-up to #2385, which fixed one overstatement in
SharedDuckDbFixture's doc comment and introduced another.The comment now reads:
That is not what
ReaderWriterLockSlimdoes.AcquireReadLocktakes a shared read lock, so readers run concurrently with each other — and this suite is overwhelmingly readers:Readers block only while a writer holds the lock or is queued for it, so the contention is bursty around the write sites (archival, compaction, CHECKPOINT, the mute and alert-history stores) rather than a flat tax on every database call.
This matters because of what the wrong reading suggests doing. "Every read and write queues on one lock" points straight at a collection fixture — which serializes the readers too, so it cannot address writer-driven contention and would cost more than it saves. The comment now says that explicitly.
It also now states plainly that nobody has measured how much of the suite's ~190–222s this is. That number is the one #2376 originally got wrong (it cited 500s from a single failed run), and leaving an unquantified cost sitting next to a proposed fix is how that happens again.
Doc comment only — no behavior change.