-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add Monitor.WaitCount + EventCounter #94810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ public static class Keywords | |
| private PollingCounter? _workingSetCounter; | ||
| private PollingCounter? _threadPoolThreadCounter; | ||
| private IncrementingPollingCounter? _monitorContentionCounter; | ||
| private IncrementingPollingCounter? _monitorWaitCounter; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it really more niche than the contention metric? In both case it blocks a thread and can cause scalability issue. Note that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Btw are you challenging that new property completely or just its addition to the event source? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am primarily challenging adding this counter to the default set. For exposing the counter as an API, we should understand what makes 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok! kouvel is not convinced either in #94264 (comment) so I'll probably close this PR. |
||
| private PollingCounter? _threadPoolQueueCounter; | ||
| private IncrementingPollingCounter? _completedItemsCounter; | ||
| private IncrementingPollingCounter? _allocRateCounter; | ||
|
|
@@ -106,6 +107,7 @@ protected override void OnEventCommand(EventCommandEventArgs command) | |
| _gen0BudgetCounter ??= new PollingCounter("gen-0-gc-budget", this, () => GC.GetGenerationBudget(0) / 1_000_000) { DisplayName = "Gen 0 GC Budget", DisplayUnits = "MB" }; | ||
| _threadPoolThreadCounter ??= new PollingCounter("threadpool-thread-count", this, () => ThreadPool.ThreadCount) { DisplayName = "ThreadPool Thread Count" }; | ||
| _monitorContentionCounter ??= new IncrementingPollingCounter("monitor-lock-contention-count", this, () => Monitor.LockContentionCount) { DisplayName = "Monitor Lock Contention Count", DisplayRateTimeScale = new TimeSpan(0, 0, 1) }; | ||
| _monitorWaitCounter ??= new IncrementingPollingCounter("monitor-wait-count", this, () => Monitor.WaitCount) { DisplayName = "Monitor Wait Count", DisplayRateTimeScale = new TimeSpan(0, 0, 1) }; | ||
| _threadPoolQueueCounter ??= new PollingCounter("threadpool-queue-length", this, () => ThreadPool.PendingWorkItemCount) { DisplayName = "ThreadPool Queue Length" }; | ||
| _completedItemsCounter ??= new IncrementingPollingCounter("threadpool-completed-items-count", this, () => ThreadPool.CompletedWorkItemCount) { DisplayName = "ThreadPool Completed Work Item Count", DisplayRateTimeScale = new TimeSpan(0, 0, 1) }; | ||
| _allocRateCounter ??= new IncrementingPollingCounter("alloc-rate", this, () => GC.GetTotalAllocatedBytes()) { DisplayName = "Allocation Rate", DisplayUnits = "B", DisplayRateTimeScale = new TimeSpan(0, 0, 1) }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -787,6 +787,7 @@ signal_monitor (gpointer mon_untyped) | |
| } | ||
|
|
||
| static gint64 thread_contentions; /* for Monitor.LockContentionCount */ | ||
| static gint64 thread_waits; /* for Monitor.WaitCount */ | ||
|
|
||
| /* If allow_interruption==TRUE, the method will be interrupted if abort or suspend | ||
| * is requested. In this case it returns -1. | ||
|
|
@@ -1340,6 +1341,8 @@ mono_monitor_wait (MonoObjectHandle obj_handle, guint32 ms, MonoBoolean allow_in | |
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want the count to be increment only when the thread yields. Not sure if that's the case here. |
||
|
|
||
| /* There's no race between unlocking mon and waiting for the | ||
| * event, because auto reset events are sticky, and this event | ||
| * is private to this thread. Therefore even if the event was | ||
|
|
@@ -1438,3 +1441,9 @@ ves_icall_System_Threading_Monitor_Monitor_get_lock_contention_count (void) | |
| { | ||
| return thread_contentions; | ||
| } | ||
|
|
||
| gint64 | ||
| ves_icall_System_Threading_Monitor_Monitor_get_wait_count (void) | ||
| { | ||
| return thread_waits; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want the count to be increment only when the thread yields. Not sure if that's the case here.