Skip to content

Conversation

@JakenVeina
Copy link
Collaborator

@JakenVeina JakenVeina commented Jun 11, 2025

Resolves #998.

The essential premise for the changes here is to extract IScheduler.Schedule() calls out of the main operator logic, specifically out of the umbrella of any lock. Because we're invoking these schedulers recursively (we form a loop by having each scheduled action finish by re-scheduling itself), and some of the core schedulers are not fully-asynchronous (including our default scheduler, TaskPoolScheduler), any locking that we do can conflict with locking being done internally within the scheduler.

In order to accomplish this refactoring, I deemed it prudent to simplify the operators and all the custom changeset assembly they were doing, to instead just use ChangeAwareList<> and ChangeAwareCache<>. The operators are MUCH more readable now, I think, and seem to have no significant change in performance, despite all the effort I put into optimizing them last time around.

We don't have a ready-to-go benchmark for the list version, but we do for the cache version:


BenchmarkDotNet v0.15.0, Windows 10 (10.0.19045.5854/22H2/2022Update)
Intel Core i7-4790K CPU 4.00GHz (Haswell), 1 CPU, 8 logical and 4 physical cores
.NET SDK 9.0.300
  [Host]     : .NET 8.0.16 (8.0.1625.21506), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.16 (8.0.1625.21506), X64 RyuJIT AVX2


Method itemCount sizeLimit Mean Error StdDev Ratio Gen0 Allocated Alloc Ratio
AddsUpdatesAndFinalization_Old 0 -1 614.5 ns 11.51 ns 10.76 ns 1.00 0.3405 1.39 KB 1.00
AddsUpdatesAndFinalization_New 0 -1 635.2 ns 11.01 ns 9.76 ns 1.03 0.3443 1.41 KB 1.01
AddsUpdatesAndFinalization_Old 0 0 623.8 ns 12.40 ns 28.23 ns 1.00 0.3557 1.45 KB 1.00
AddsUpdatesAndFinalization_New 0 0 663.7 ns 13.27 ns 33.53 ns 1.07 0.3443 1.41 KB 0.97
AddsUpdatesAndFinalization_Old 0 1 650.6 ns 12.69 ns 11.87 ns 1.00 0.4072 1.66 KB 1.00
AddsUpdatesAndFinalization_New 0 1 652.8 ns 12.96 ns 25.27 ns 1.00 0.3443 1.41 KB 0.85
AddsUpdatesAndFinalization_Old 1 1 895.3 ns 17.65 ns 26.42 ns 1.00 0.4587 1.88 KB 1.00
AddsUpdatesAndFinalization_New 1 1 937.5 ns 18.44 ns 34.18 ns 1.05 0.4454 1.82 KB 0.97
AddsUpdatesAndFinalization_Old 10 -1 2,839.7 ns 56.10 ns 130.01 ns 1.00 0.7172 2.94 KB 1.00
AddsUpdatesAndFinalization_New 10 -1 2,714.7 ns 38.81 ns 34.41 ns 0.96 1.0262 4.2 KB 1.43
AddsUpdatesAndFinalization_Old 10 1 3,715.1 ns 70.61 ns 155.00 ns 1.00 0.8392 3.43 KB 1.00
AddsUpdatesAndFinalization_New 10 1 3,338.1 ns 65.88 ns 147.34 ns 0.90 0.9766 4 KB 1.17
AddsUpdatesAndFinalization_Old 10 5 2,926.3 ns 55.60 ns 89.79 ns 1.00 0.8888 3.64 KB 1.00
AddsUpdatesAndFinalization_New 10 5 3,056.8 ns 61.06 ns 91.38 ns 1.05 1.0490 4.3 KB 1.18
AddsUpdatesAndFinalization_Old 10 10 3,253.0 ns 75.83 ns 223.58 ns 1.00 1.0300 4.21 KB 1.00
AddsUpdatesAndFinalization_New 10 10 3,213.9 ns 64.25 ns 96.17 ns 0.99 1.0490 4.3 KB 1.02
AddsUpdatesAndFinalization_Old 100 -1 23,958.4 ns 640.21 ns 1,877.61 ns 1.01 4.5776 18.79 KB 1.00
AddsUpdatesAndFinalization_New 100 -1 23,428.2 ns 545.10 ns 1,598.69 ns 0.98 7.3547 30.06 KB 1.60
AddsUpdatesAndFinalization_Old 100 10 33,166.9 ns 657.50 ns 1,134.16 ns 1.00 4.8828 20.06 KB 1.00
AddsUpdatesAndFinalization_New 100 10 28,476.1 ns 552.29 ns 698.47 ns 0.86 6.5308 26.7 KB 1.33
AddsUpdatesAndFinalization_Old 100 50 25,202.2 ns 521.92 ns 1,538.88 ns 1.00 6.2866 25.79 KB 1.00
AddsUpdatesAndFinalization_New 100 50 24,702.4 ns 490.95 ns 922.12 ns 0.98 7.5073 30.66 KB 1.19
AddsUpdatesAndFinalization_Old 100 100 25,535.0 ns 505.27 ns 1,020.66 ns 1.00 6.9580 28.42 KB 1.00
AddsUpdatesAndFinalization_New 100 100 24,019.2 ns 389.97 ns 478.91 ns 0.94 7.5073 30.66 KB 1.08
AddsUpdatesAndFinalization_Old 1000 -1 203,184.0 ns 3,196.13 ns 2,833.29 ns 1.00 32.9590 135.16 KB 1.00
AddsUpdatesAndFinalization_New 1000 -1 216,197.1 ns 4,274.04 ns 10,157.70 ns 1.06 64.6973 265.3 KB 1.96
AddsUpdatesAndFinalization_Old 1000 100 351,317.4 ns 6,996.38 ns 17,030.16 ns 1.00 45.4102 186.45 KB 1.00
AddsUpdatesAndFinalization_New 1000 100 292,328.5 ns 6,190.91 ns 18,059.18 ns 0.83 62.0117 254.59 KB 1.37
AddsUpdatesAndFinalization_Old 1000 500 249,825.5 ns 5,440.54 ns 15,956.15 ns 1.00 58.5938 240.85 KB 1.00
AddsUpdatesAndFinalization_New 1000 500 241,253.9 ns 4,530.95 ns 10,227.11 ns 0.97 65.9180 269.48 KB 1.12
AddsUpdatesAndFinalization_Old 1000 1000 257,805.0 ns 5,734.86 ns 16,819.37 ns 1.00 66.4063 272.36 KB 1.00
AddsUpdatesAndFinalization_New 1000 1000 240,498.3 ns 4,674.00 ns 11,465.41 ns 0.94 65.9180 269.48 KB 0.99

I also had to come up with a way to manage test parallelization, since I have tests that are time-sensitive, to try and detect deadlocks, and they were failing due to thread/task starvation, when running in parallel with the rest of the test suite. Unfortunately, controlling parallelism within xUnit not straightforward. I think the pattern I came up with makes sense, but I plan to look into upgrading to xUnit v3 in the near future, and I that may render this setup obsolete.

Copy link
Collaborator

@RolandPheasant RolandPheasant left a comment

Choose a reason for hiding this comment

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

LGTM

@JakenVeina JakenVeina merged commit 7295286 into main Jun 18, 2025
1 check passed
@JakenVeina JakenVeina deleted the issues/998 branch June 18, 2025 06:45
@github-actions
Copy link

github-actions bot commented Jul 3, 2025

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Deadlock using expireAfter

3 participants