Implement APIs for some threading metrics (CoreRT) #7066

kouvel · 2019-02-21T20:47:42Z

API review: https://github.com/dotnet/corefx/issues/35500
May depend on Implement APIs for some threading metrics (CoreCLR) coreclr#22754

kouvel · 2019-02-21T20:54:07Z

There is a slightly noticeable decrease in throughput of local work items queued to the thread pool in raw overhead per work item. It is very small and when a very short delay is added to the work item the difference is almost not measurable, so it should be acceptably small.

filipnavara · 2019-02-22T00:39:00Z

Fixed Timer implementation on Unixes. Previously there was only ever one timer request from the upper-level implementation and that is not the case anymore, so the lower-level "app domain timer" implementation needed to handle multiple timer requests.

I am not sure I understand your motivation for the change. I believe the whole point was to split the queues and process them separately in separate threads. Now you are trying to undo that at lower level. Am I missing something?

src/Native/Runtime/thread.cpp

kouvel · 2019-02-22T03:53:08Z

I am not sure I understand your motivation for the change. I believe the whole point was to split the queues and process them separately in separate threads. Now you are trying to undo that at lower level. Am I missing something?

Only one thread was created and the TimerQueue ID was being ignored. There isn't much benefit to using multiple threads, the goal of the original change was to split the queues such that each queue gets a separate callback on the thread pool and each queue has its own lock so they can operate in parallel, that benefit is still there. CoreCLR also uses only one timer thread to serve all timer requests from TimerQueue.

filipnavara · 2019-02-22T06:49:50Z

Only one thread was created and the TimerQueue ID was being ignored.

The ID was not ignored, it just was not necessary since the instance variables could be used in the pure managed code implementation. The TimerQueue objects were instantiated here:

corert/src/System.Private.CoreLib/shared/System/Threading/Timer.cs

Lines 46 to 54 in a8cb34b

    
           private static TimerQueue[] CreateTimerQueues() 
        
           { 
        
               var queues = new TimerQueue[Environment.ProcessorCount]; 
        
               for (int i = 0; i < queues.Length; i++) 
        
               { 
        
                   queues[i] = new TimerQueue(i); 
        
               } 
        
               return queues; 
        
           }

Each TimerQueue instance then already had a reference to it's own queue in the instance variables and SetTimer in Timer.Unix.cs started the thread for that queue.

There isn't much benefit to using multiple threads, the goal of the original change was to split the queues such that each queue gets a separate callback on the thread pool and each queue has its own lock so they can operate in parallel, that benefit is still there.

Ok, for reference: dotnet/coreclr#14527.

CoreCLR also uses only one timer thread to serve all timer requests from TimerQueue.

This is the part I missed. I saw the code in AppDomainTimerNative::CreateAppDomainTimer which lead me to believe that more threads are actually created. However, ultimately it's all folded back into one thread in
https://github.com/dotnet/coreclr/blob/eb1aa094dbd32f98fd4ce5dead9f27f9c78a32c5/src/vm/win32threadpool.cpp#L4429-L4521.

I am not sure if the change back to one thread is actually beneficial, but if it matches CoreCLR then it's probably ok. Would be nice to have a benchmark, there is a simple one in dotnet/coreclr#20302.

kouvel · 2019-02-22T07:46:07Z

SetTimer in Timer.Unix.cs started the thread for that queue

It only started one thread for all TimerQueues because s_timerEvent is static, actually the starting of the thread also had race issues (which were introduced after multiple queues were introduced) that may cause multiple threads to be started, possibly multiple threads for the same queue, and very possibility not starting threads for other queues. See the test case I added in Threading.cs, which fails reliably before the fix.

It could be fixed with fewer changes and more threads (changing some static variables to instance variables, and some such), and it would still be unfortunate to have 32 timer threads on a 32-core machine all for just the purpose of a very small improvement at best. Timers can only request to tick once per millisecond, which is already long, and the resolution is actually about 15+ ms on Windows, any gain with having multiple threads serving those timer requests would be squashed just by the timer resolution.

filipnavara · 2019-02-22T08:19:30Z

It only started one thread for all TimerQueues because s_timerEvent is static

Ah, that is a bug I missed. It should have been changed to an instance variable. It is called from under lock (FireNextTimers -> lock (this) -> EnsureTimerFiresBy -> SetTimer), so other race conditions were not really a concern there.

it would still be unfortunate to have 32 timer threads on a 32-core machine all for just the purpose of a very small improvement at best.

I'm not convinced that the improvement is so small for a case where the timers are heavily used. However, I do believe that for the common case it will likely be overkill to have multiple threads.

There are couple other small concerns I have with the change so it would have been nice if it was split off into separate PR.

src/System.Private.CoreLib/src/System/Threading/Timer.Unix.cs

kouvel · 2019-03-07T01:09:05Z

The RT implementation in latest commit is broken, will fix and retest

kouvel · 2019-03-07T08:27:54Z

Updated perf numbers are here

kouvel · 2019-03-18T23:30:37Z

Ping for review please

kouvel · 2019-03-18T23:31:04Z

@dotnet-bot test OSX10.12 Debug and CoreFX tests

kouvel · 2019-03-20T20:05:35Z

The API review was approved. Couple of pending items is:

Whether to expose PendingLocalWorkItemCount and PendingGlobalWorkItemCount. I have asked for more info on whether they would be useful, by default I'll go ahead and remove them
Whether we want to expose CompletedWorkItemCount at all.
- I think that's the most expensive one to track of the lot, as it involves a bit of extra work per work item
- It is possible to mostly eliminate the extra cost for managed work items in CoreCLR and CoreRT. Work items that may have some extra cost are timer callbacks in CoreRT Windows and IO completion work items in CoreCLR Windows.
- The info would enable easily determining work item throughput and whether the thread pool is stalled

…ad (#7071) * Change Timer implementation on Unixes to use only one scheduling thread - Separated from #7066 * Address feedback from #7066 * Remove reference to s_lock * Reduce work inside lock * Move _id * Fix duplicate timers in scheduled timer list, move info to TimerQueue

…ad (dotnet#7071) * Change Timer implementation on Unixes to use only one scheduling thread - Separated from dotnet/corert#7066 * Address feedback from dotnet/corert#7066 * Remove reference to s_lock * Reduce work inside lock * Move _id * Fix duplicate timers in scheduled timer list, move info to TimerQueue Signed-off-by: dotnet-bot <[email protected]>

…ad (mono#7071) * Change Timer implementation on Unixes to use only one scheduling thread - Separated from dotnet/corert#7066 * Address feedback from dotnet/corert#7066 * Remove reference to s_lock * Reduce work inside lock * Move _id * Fix duplicate timers in scheduled timer list, move info to TimerQueue Signed-off-by: dotnet-bot <[email protected]>

…ad (#7071) * Change Timer implementation on Unixes to use only one scheduling thread - Separated from dotnet/corert#7066 * Address feedback from dotnet/corert#7066 * Remove reference to s_lock * Reduce work inside lock * Move _id * Fix duplicate timers in scheduled timer list, move info to TimerQueue Signed-off-by: dotnet-bot <[email protected]>

…ad (dotnet#7071) * Change Timer implementation on Unixes to use only one scheduling thread - Separated from dotnet/corert#7066 * Address feedback from dotnet/corert#7066 * Remove reference to s_lock * Reduce work inside lock * Move _id * Fix duplicate timers in scheduled timer list, move info to TimerQueue Signed-off-by: dotnet-bot <[email protected]>

…ad (#7071) * Change Timer implementation on Unixes to use only one scheduling thread - Separated from dotnet/corert#7066 * Address feedback from dotnet/corert#7066 * Remove reference to s_lock * Reduce work inside lock * Move _id * Fix duplicate timers in scheduled timer list, move info to TimerQueue Signed-off-by: dotnet-bot <[email protected]>

…ad (dotnet#7071) * Change Timer implementation on Unixes to use only one scheduling thread - Separated from dotnet/corert#7066 * Address feedback from dotnet/corert#7066 * Remove reference to s_lock * Reduce work inside lock * Move _id * Fix duplicate timers in scheduled timer list, move info to TimerQueue Signed-off-by: dotnet-bot <[email protected]>

kouvel · 2019-04-02T22:09:43Z

The perf difference from adding CompletedWorkItemCount seems to be very small, so for now I'll consider that it can reasonably be added

kouvel · 2019-04-02T22:12:51Z

I have updated to remove local/global versions of PendingWorkItemCount. They can be added in the future if they would be useful. I think all three PRs (CoreCLR, CoreRT, and CoreFX) are now ready for review. @jkotas / @stephentoub, would you be able to take a look?

jkotas · 2019-04-02T23:19:25Z

src/System.Private.CoreLib/src/System/Threading/ThreadBooleanCounter.cs

+        {
+            get
+            {
+                // Make sure up-to-date thread-local node state is visible to this thread


Should not be needed.

jkotas · 2019-04-02T23:19:36Z

src/System.Private.CoreLib/src/System/Threading/ThreadInt64PersistentCounter.cs

+        {
+            get
+            {
+                // Make sure up-to-date thread-local node state is visible to this thread


Should not be needed.

jkotas · 2019-04-02T23:38:54Z

src/System.Private.CoreLib/shared/System/Threading/ThreadLocal.cs

+            LinkedSlot linkedSlot = _linkedSlot;
            int id = ~_idComplement;
-            if (id == -1)
+            if (id == -1 || linkedSlot == null)


Is this fixing a bug when _linkedSlot? Do we need a test for it?

Seems like a possibility that after Dispose is called and another thread calls this method, because there are no restrictions on the loads here, the other thread could read an old value for _idComplement (where id != 0) and a new value for _linkedSlot (== null), then it would throw NullReferenceException instead of ObjectDisposedException. I didn't see anything that would prevent that, and thought it would be difficult to repro but it seems to be fairly easy, will add a test to the CoreFX PR.

jkotas

Thanks

- API review: https://github.com/dotnet/corefx/issues/35500 - May depend on dotnet/coreclr#22754

…etect thread exit

kouvel · 2019-04-23T00:20:29Z

Rebased

jkotas · 2019-04-23T01:07:48Z

shared\System\Threading\ThreadLocal.cs(457,37): error CS8600: Converting null literal or possible null value to non-nullable type.
shared\System\Threading\ThreadLocal.cs(466,31): error CS8600: Converting null literal or possible null value to non-nullable type.
shared\System\Threading\ThreadLocal.cs(466,82): error CS8600: Converting null literal or possible null value to non-nullable type.
shared\System\Threading\ThreadLocal.cs(485,41): error CS8600: Converting null literal or possible null value to non-nullable type.
shared\System\Threading\ThreadLocal.cs(493,35): error CS8600: Converting null literal or possible null value to non-nullable type.
shared\System\Threading\ThreadLocal.cs(493,86): error CS8600: Converting null literal or possible null value to non-nullable type.

* Implement APIs for some threading metrics (CoreRT) - API review: https://github.com/dotnet/corefx/issues/35500 - May depend on dotnet#22754 * Use thread-locals for counting, use finalizer instead of runtime to detect thread exit * Don't let the count properties throw OOM * Remove some flushes Signed-off-by: dotnet-bot <[email protected]>

* Implement APIs for some threading metrics (CoreRT) - API review: https://github.com/dotnet/corefx/issues/35500 - May depend on dotnet/coreclr#22754 * Use thread-locals for counting, use finalizer instead of runtime to detect thread exit * Don't let the count properties throw OOM * Remove some flushes Signed-off-by: dotnet-bot <[email protected]>

* Implement APIs for some threading metrics (CoreRT) - API review: https://github.com/dotnet/corefx/issues/35500 - May depend on #22754 * Use thread-locals for counting, use finalizer instead of runtime to detect thread exit * Don't let the count properties throw OOM * Remove some flushes Signed-off-by: dotnet-bot <[email protected]>

* Expose and test APIs for some threading metrics (CoreFX) - API review: https://github.com/dotnet/corefx/issues/35500 - Depends on dotnet/coreclr#22754, dotnet/corert#7066 * Separate and expose pending local vs global work item count * Remove local/global variants of PendingWorkItemCount * Remove unrelated test * Add test for a fix to ThreadLocal.Values property throwing NullReferenceException when disposed Fix is in dotnet/corert#7066 * Fix build * Fix test * Add API compat baselines for uapaot * Fix test * Use RemoteExecutor for MetricsTest * Address feedback

kouvel added the area-System.Threading label Feb 21, 2019

kouvel added this to the Preview milestone Feb 21, 2019

kouvel self-assigned this Feb 21, 2019

kouvel requested a review from stephentoub February 21, 2019 20:47

kouvel added the * NO MERGE * label Feb 21, 2019

kouvel mentioned this pull request Feb 21, 2019

Expose and test APIs for some threading metrics (CoreFX) dotnet/corefx#35502

Closed

jkotas reviewed Feb 22, 2019

View reviewed changes

src/Native/Runtime/thread.cpp Outdated Show resolved Hide resolved

filipnavara reviewed Feb 22, 2019

View reviewed changes

src/System.Private.CoreLib/src/System/Threading/Timer.Unix.cs Show resolved Hide resolved

filipnavara reviewed Feb 22, 2019

View reviewed changes

src/System.Private.CoreLib/src/System/Threading/Timer.Unix.cs Outdated Show resolved Hide resolved

filipnavara reviewed Feb 22, 2019

View reviewed changes

src/System.Private.CoreLib/src/System/Threading/Timer.Unix.cs Outdated Show resolved Hide resolved

filipnavara mentioned this pull request Feb 22, 2019

Fix timer firing bug introduced in commit e652d4d (#6905 / #6898) #7069

Merged

kouvel mentioned this pull request Feb 22, 2019

Change Timer implementation on Unixes to use only one scheduling thread #7071

Merged

kouvel changed the title ~~Implement APIs for some threading metrics (CoreRT), fix Timer on Unixes~~ Implement APIs for some threading metrics (CoreRT) Mar 6, 2019

kouvel removed the * NO MERGE * label Mar 30, 2019

jkotas reviewed Apr 2, 2019

View reviewed changes

jkotas approved these changes Apr 2, 2019

View reviewed changes

kouvel added the * NO MERGE * label Apr 5, 2019

kouvel removed the * NO MERGE * label Apr 20, 2019

Koundinya Veluri added 6 commits April 22, 2019 17:16

Implement APIs for some threading metrics (CoreRT)

0b9eff5

- API review: https://github.com/dotnet/corefx/issues/35500 - May depend on dotnet/coreclr#22754

Use thread-locals for counting, use finalizer instead of runtime to d…

138d4be

…etect thread exit

Fixes

0f56d1b

Don't let the count properties throw OOM

e88470d

Remove some flushes

509e620

Remove flushes

7a78005

Fix build

0441b7b

jkotas merged commit d068307 into dotnet:master Apr 23, 2019

filipnavara mentioned this pull request Jun 6, 2019

[netcore]Implement Monitor.LockContentionCount mono/mono#14828

Closed

kouvel deleted the ThreadMetricsRt branch April 3, 2023 21:12

Implement APIs for some threading metrics (CoreRT) #7066

Implement APIs for some threading metrics (CoreRT) #7066

Uh oh!

Conversation

kouvel commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kouvel commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filipnavara commented Feb 22, 2019

Uh oh!

Uh oh!

kouvel commented Feb 22, 2019

Uh oh!

filipnavara commented Feb 22, 2019

Uh oh!

kouvel commented Feb 22, 2019

Uh oh!

filipnavara commented Feb 22, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kouvel commented Mar 7, 2019

Uh oh!

kouvel commented Mar 7, 2019

Uh oh!

kouvel commented Mar 18, 2019

Uh oh!

kouvel commented Mar 18, 2019

Uh oh!

kouvel commented Mar 20, 2019

Uh oh!

kouvel commented Apr 2, 2019

Uh oh!

kouvel commented Apr 2, 2019

Uh oh!

jkotas Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

jkotas Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

jkotas Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

kouvel Apr 3, 2019 • 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!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

kouvel commented Apr 23, 2019

Uh oh!

jkotas commented Apr 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kouvel commented Feb 21, 2019 •

edited

Loading

kouvel commented Feb 21, 2019 •

edited

Loading

kouvel Apr 3, 2019 •

edited

Loading