Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-collections |
0fac552 to
a98f114
Compare
a98f114 to
5dd0bd0
Compare
5dd0bd0 to
ff05973
Compare
ff05973 to
31ffa76
Compare
ApproachThe current implementation walks the requested number of nodes before attempting the CAS. Under contention, the head can change while we are traversing the list, making the work we have already done more likely to be discarded by a failed CAS. This change periodically checks whether The frequency of these checks is scaled based on count, so smaller pops perform fewer checks while large pops check often enough to avoid spending a significant amount of time traversing a stale chain. int shift = Numerics.BitOperations.Log2(int.MaxValue / (uint)count);
int checkHeadMask = (1 << shift) - 1;Benchmarks ResultsTryPopCount = 3
TryPopCount = 10
TryPopCount = 100
TryPopCount = 1,000
TryPopCount = 10,000
TryPopCount = 100,000
TryPopCount = 1,000,000
Note The results show a clear trade-off: the additional Is this trade-off acceptable? Open QuestionI also noticed that skipping the CAS when I'm not sure why this happens. Is there a reason we should still attempt the CAS in this case? @jkotas Since you previously looked at the earlier validation approach, I'd also appreciate your thoughts on the new approach and the open question above. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The early-abort path performs a guaranteed-failing CAS, while ordinary TryPop gains unnecessary arithmetic overhead.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Optimizes ConcurrentStack<T>.TryPopRange by detecting contention during long range traversals.
Changes:
- Adds adaptive head-check intervals.
- Aborts traversal when the stack head changes.
| File | Description |
|---|---|
ConcurrentStack.cs |
Adds contention detection to range popping. |
| if ((nodesCount & checkHeadMask) == 0) | ||
| { | ||
| if (head != _head) | ||
| { | ||
| break; |
There was a problem hiding this comment.
I did some additional experiments around skipping the CAS after detecting that _head has changed. The results are mixed for smaller ranges:
| TryPopCount | Threads | Baseline | CAS kept | CAS skipped |
|---|---|---|---|---|
| 3 | 16 | 24.08 μs | 24.46 μs | 25.29 μs |
| 10 | 16 | 22.67 μs | 20.64 μs | 21.35 μs |
| 100 | 16 | 43.86 μs | 45.23 μs | 42.65 μs |
| 1,000 | 16 | 69.38 μs | 68.68 μs | 66.66 μs |
For 100,000+, skipping the CAS caused a substantial regression under contention, reaching seconds per iteration. These cases also take significantly longer to benchmark.
@tannergooding, I'd be interested in your thoughts on this, and whether there is a way to avoid the CAS instruction in this path.
This is not unusual behavior for spin locks under contention when they are protecting an operation that is non-trivial. My guess is that this behavior would vary across machines or with small modifications of the microbenchmark. |
| @@ -586,6 +586,8 @@ private int TryPopCore(int count, out Node? poppedHead) | |||
| Node? head; | |||
| Node next; | |||
| int backoff = 1; | |||
| int shift = Numerics.BitOperations.Log2(int.MaxValue / (uint)count); | |||
| int checkHeadMask = (1 << shift) - 1; | |||
There was a problem hiding this comment.
Is there some rationale behind this formula?
If I am reading this correctly, these additional checks are only going to kick for count >30k. For example, when count = 10_000, checkHeadMask is going to be 131071 and so we won't execute any additional checks. So it is surprising that you are able to measure an improvement for 10_000.
There was a problem hiding this comment.
You're right — for count = 10,000, the formula does not actually introduce any additional _head checks.
The rationale was to make the check interval adaptive to count— checking _head more frequently as range size grows to prevent heavy traversal on a stale chain, while keeping checks infrequent for smaller counts to minimize overhead. I intentionally used a heuristic rather than hardcoded thresholds, derived purely from experimentation.
Regarding the improvements observed at smaller counts (like 10,000), the repeated runs don't show a consistent benefit, so I don't consider them meaningful evidence for the heuristic:
TryPopCount = 10,000 (After):
| ParallelThreads | Run 1 | Run 2 | Run 3 |
|---|---|---|---|
| 0 | 179.8 μs | 198.5 μs | 176.7 μs |
| 1 | 287.0 μs | 333.0 μs | 278.9 μs |
| 2 | 620.3 μs | 590.9 μs | 563.7 μs |
| 5 | 1,069.2 μs | 1,189.5 μs | 1,203.9 μs |
| 8 | 666.9 μs | 1,367.8 μs | 1,431.1 μs |
| 16 | 771.1 μs | 1,446.7 μs | 1,496.8 μs |
Baseline (Before):
| ParallelThreads | Run 1 | Run 2 | Run 3 |
|---|---|---|---|
| 0 | 176.2 μs | 175.5 μs | 175.8 μs |
| 1 | 283.0 μs | 285.4 μs | 282.6 μs |
| 2 | 664.8 μs | 690.0 μs | 584.8 μs |
| 5 | 1,181.9 μs | 1,157.8 μs | 1,185.0 μs |
| 8 | 1,438.0 μs | 1,215.8 μs | 1,193.2 μs |
| 16 | 1,539.0 μs | 1,946.6 μs | 1,561.5 μs |
I don't consider small-count improvements meaningful evidence for the heuristic; the main value of this PR lies in large ranges under high contention, where stopping stale traversals can prevent severe performance degradation.


Try to improve
ConcurrentStack.TryPopRangeperformance under contention described in #100083.Benchmark Setup
Environment