Add Monitor.WaitCount + EventCounter #94810
Conversation
ghost
added
community-contribution
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @mangod9 |
| bool success = false; | ||
| try | ||
| { | ||
| Interlocked.Increment(ref _waitCount); |
There was a problem hiding this comment.
I want the count to be increment only when the thread yields. Not sure if that's the case here.
|
|
||
| LOCK_DEBUG (g_message ("%s: (%d) Unlocked %p lock %p", __func__, id, obj, mon)); | ||
|
|
||
| mono_atomic_inc_i64 (&thread_waits); |
There was a problem hiding this comment.
I want the count to be increment only when the thread yields. Not sure if that's the case here.
| private PollingCounter? _workingSetCounter; | ||
| private PollingCounter? _threadPoolThreadCounter; | ||
| private IncrementingPollingCounter? _monitorContentionCounter; | ||
| private IncrementingPollingCounter? _monitorWaitCounter; |
There was a problem hiding this comment.
This is a very niche metric. I do not think it belongs to this default set of runtime perf counters.
There was a problem hiding this comment.
Is it really more niche than the contention metric? In both case it blocks a thread and can cause scalability issue. Note that Monitor.Wait is used by synchronous I/O operations, ManualEventSlim, Task.Wait, ...
There was a problem hiding this comment.
The interpretation of this counter depends heavily on how all Monitor.Wait are used by the app. For example - if this counter grows by 100 or 1000 per second, does the app have a scalability problem?
Some Monitor.Waits are fine and some can be scalability problems. I do not think you can tell in general. It is what makes it a bad candidate for on-by-default counter.
There was a problem hiding this comment.
The interpretation of this counter depends heavily on how all Monitor.Wait are used by the app
Isn't that true for most runtime metrics? For example if I see an increase of gen 0 collections, I'll have to get more info about the app to determine if that's an issue.
If I see a spike of latency and I'm able to correlate it with a spike of Monitor.Wait that could point out a thread starvation. Of course that doesn't prove anything but it gives a lead, and it could be confirmed by collecting the event proposed in #94737.
There was a problem hiding this comment.
Most default runtime metrics have a healthy range/rate that will hold for most apps. I think it will be hard to come up with a general healthy range/rate for this one.
There was a problem hiding this comment.
I do not think it belongs to this default set of runtime perf counters
Btw are you challenging that new property completely or just its addition to the event source?
There was a problem hiding this comment.
I am primarily challenging adding this counter to the default set.
For exposing the counter as an API, we should understand what makes Monitor.Wait special that warrants adding the counter for it. Let me rephrase the justification for adding this counter: An app started to misbehave that lead to Monitor.Wait being called much more frequently than before. A counter that measures how often Monitor.Wait gets called can alert to the situation, thus we should add a counter for it. The problem with this justification is that it does not scale. There are many other APIs that will lead to thread pool starvation when called on threadpool threads a lot. It is not reasonable to add a counter for each of them.
We have multiple tools for diagnosing threadpool starvation. The most basic one is counter for threadpool threads. If this counter starts growing suddenly, it is a sign of threadpool starvation. There are other more advanced tools, like the ThreadAdjustmentReasonMap.Starvation event emitted by threadpool. If the current tools are not good enough, we should think about ways to make them work for any root cause of the threadpool starvation.
There was a problem hiding this comment.
Ok! kouvel is not convinced either in #94264 (comment) so I'll probably close this PR.
2 participants
Context: #94264
Conflicts with first PR: #94737