Some more perf tweaks to ThreadPool by stephentoub · Pull Request #9279 · dotnet/coreclr

stephentoub · 2017-02-02T20:08:05Z

Main changes:

The work-stealing system was storing all of the local lists in a sparse array. Any time a thread went looking for work, it would pick a random place to start in the array and iterate through all of the cells. But the array often was much larger than the number of lists it contained, because it a) grows doubling each time, and b) never shrinks when threads are removed, so all of the iterating needed to check for null on every cell. And all of the reads were done with Volatile.Read, to account for potentially concurrent writes as threads were added/removed. However, the addition/removal of thread-local lists is actually relatively rare (even when the thread pool takes threads in and out of service, it's not generally not allocating new threads or destroying old ones, just keeping them in reserve on the side for a while). Instead, we can just use an immutable array that's always the right size.
All of the old StackCrawlMark was still cluttering up {Unsafe}QueueUserWorkItem and preventing those functions and helpers from getting inlined. I've removed that goo.
Random.Next is used to determine where a thread should start looking for work to steal, and it was showing up in a trace as a few percentage points. I've replaced it with a faster routine.
Cleaned up some empty try blocks and the associated code around it, and helped a few other functions that were showing up in traces be more inlineable.

jkotas · 2017-02-02T20:36:22Z

@@ -534,81 +536,66 @@ internal static bool Dispatch()
                    // Dequeue and EnsureThreadRequested must be protected from ThreadAbortException.  


Comment not applicable anymore

jkotas · 2017-02-02T20:37:22Z

    }

+    // Simple random number generator. We don't need great randomness, we just need a little and for it to be fast.
+    internal sealed class FastRandom // xorshift prng


Changed to a struct

stephentoub · 2017-02-02T22:53:30Z

@jkotas, I've not seen this error before (in the arm Cross Debug Build leg):

error C3859: virtual memory range for PCH exceeded; please recompile with a command line option of '-Zm58' or greater [D:\j\workspace\arm_cross_deb---74b8368e\bin\obj\Windows_NT.arm.Debug\crossgen\src\vm\crossgen\cee_crossgen.vcxproj]

I assume this has nothing to do with my PR?

jkotas · 2017-02-02T22:57:12Z

I do not think it has anything to do with your PR. I have not seen it before.

stephentoub · 2017-02-02T23:13:26Z

@dotnet-bot test Windows_NT arm Cross Debug Build please

benaadams · 2017-02-03T00:19:05Z

+            _w = _w ^ (_w >> 19) ^ (t ^ (t >> 8));
+
+            int r = (int)_w & int.MaxValue;
+            return (int)(r * (1.0 / ((double)int.MaxValue + 1)) * maxValue);


As much as idiv bothers me; I wonder what the effect of casting to double doing math and casting back; since vectors are increasingly in play; may be better just to % maxValue? (with early exit for maxValue == 0)

I'll switch to mod. Looks like it might be 1-2% faster, but regardless it's simpler.

I still wonder if light coordination between the threads using interlocked might be better than random to avoid collisions

As in rather than
int i = tl.random.Next(c);
do
int i = NextCounter.Next(c);

with it being something like https://gist.github.com/benaadams/9a66811bc54e7126a9cd8c181bbe6f53

Maybe something to look at in future.

Thanks, Ben. Please let me know what you find (if you look).

benaadams · 2017-02-03T00:19:51Z

LGTM

The current data structure used to store the list of local work-stealing queues is maintains a sparse array, where entries can be null, and where removals null out entries in an active array that could be being read by another thread concurrently. This necessitates that threads looking for work need to use volatile reads and null checks on each element. Further, because the array doubles in size when it grows, and never shrinks, we often end up having many empty slots that threads need to look at as they're looking for work. It's actually relatively rare for threads to come and go. While the thread pool does take threads in and out of service, it only rarely actually terminates a thread or asks the OS for a new one, so it's relatively rare that threads are added/removed from the list. Given that, we can simply use immutable arrays, creating a new array of the exact right size whenever a thread is added or removed. Then iteration can be done without volatile reads and without null checks, because the contents of the array being read through won't change and won't ever be null.

And clean up the code around it.

Allow more methods to be inlined

We need some randomness, but it doesn't need to be particularly good, just fast. A simple xorshift prng is 3-4x faster than Random, in particular without multiple virtual method calls that don't get inlined. It's also a bit lighter in terms of memory (though there's only one of these per thread, so it doesn't impact much).

- Change LocalPop to be parameterless and return the IThreadPoolWorkItem rather than returning a bool and having the IThreadPoolWorkItem in an out arg. - Refactor LocalPop slightly to have an inlineable upfront check for whether the local queue is empty, and only call the non-inlineable method if there's something that could potentially be popped

Some more perf tweaks to ThreadPool Commit migrated from dotnet/coreclr@cfd28c5

dnfclas added the cla-already-signed label Feb 2, 2017

jkotas reviewed Feb 2, 2017

View reviewed changes

stephentoub force-pushed the more_tp_perf branch from b875c42 to dd13b29 Compare February 2, 2017 22:25

jkotas approved these changes Feb 2, 2017

View reviewed changes

benaadams reviewed Feb 3, 2017

View reviewed changes

stephentoub force-pushed the more_tp_perf branch from dd13b29 to e361700 Compare February 3, 2017 02:44

stephentoub added 9 commits February 3, 2017 09:25

Reorder usings per previous PR feedback

6294a0d

Remove unnecessary try/finally blocks

c3cefb7

And clean up the code around it.

Remove StackCrawlMark goo from {Unsafe}QueueUserWorkItem

d41bffb

Allow more methods to be inlined

Enable inlining of EnsureVMInitialized

a0d078a

Address PR feedback

43985a2

Allow Dequeue to avoid TrySteal if there's nothing to steal

b185fe7

stephentoub force-pushed the more_tp_perf branch from e361700 to 70aae70 Compare February 3, 2017 14:25

stephentoub merged commit cfd28c5 into dotnet:master Feb 3, 2017

stephentoub deleted the more_tp_perf branch February 3, 2017 16:10

jkotas mentioned this pull request Feb 12, 2017

Port CoreCLR threading perf improvements dotnet/corert#2640

Open

kouvel mentioned this pull request Jul 22, 2017

Port Perf fixes over from CoreCLR. dotnet/corert#4150

Merged

karelz modified the milestone: 2.0.0 Aug 28, 2017

kouvel added area-System.Threading netfx-port-consider labels Sep 15, 2017

benaadams mentioned this pull request Dec 29, 2018

[WIP] Lazy alloc ThreadPool local queues #21695

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#9279 from stephentoub/more_tp_perf

1c3d055

Some more perf tweaks to ThreadPool Commit migrated from dotnet/coreclr@cfd28c5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some more perf tweaks to ThreadPool#9279

Some more perf tweaks to ThreadPool#9279
stephentoub merged 9 commits intodotnet:masterfrom
stephentoub:more_tp_perf

stephentoub commented Feb 2, 2017

Uh oh!

jkotas Feb 2, 2017

Uh oh!

stephentoub Feb 2, 2017

Uh oh!

jkotas Feb 2, 2017

Uh oh!

stephentoub Feb 2, 2017

Uh oh!

stephentoub commented Feb 2, 2017

Uh oh!

jkotas commented Feb 2, 2017

Uh oh!

stephentoub commented Feb 2, 2017

Uh oh!

benaadams Feb 3, 2017 •

edited

Loading

Uh oh!

stephentoub Feb 3, 2017

Uh oh!

benaadams Feb 3, 2017 •

edited

Loading

Uh oh!

stephentoub Feb 3, 2017 •

edited

Loading

Uh oh!

benaadams commented Feb 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

		@@ -534,81 +536,66 @@ internal static bool Dispatch()
		// Dequeue and EnsureThreadRequested must be protected from ThreadAbortException.

Conversation

stephentoub commented Feb 2, 2017

Uh oh!

jkotas Feb 2, 2017

Choose a reason for hiding this comment

Uh oh!

stephentoub Feb 2, 2017

Choose a reason for hiding this comment

Uh oh!

jkotas Feb 2, 2017

Choose a reason for hiding this comment

Uh oh!

stephentoub Feb 2, 2017

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Feb 2, 2017

Uh oh!

jkotas commented Feb 2, 2017

Uh oh!

stephentoub commented Feb 2, 2017

Uh oh!

benaadams Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Feb 3, 2017

Choose a reason for hiding this comment

Uh oh!

benaadams Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Feb 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

benaadams Feb 3, 2017 •

edited

Loading

benaadams Feb 3, 2017 •

edited

Loading

stephentoub Feb 3, 2017 •

edited

Loading