From 649e4c6edae752225a90dedf9983e39d226aae7c Mon Sep 17 00:00:00 2001 From: "Evgeny.Terekhin" Date: Fri, 2 Aug 2024 11:34:59 +0200 Subject: [PATCH 1/3] Reset IncrementingPollingCounter on Timer thread --- .../Diagnostics/Tracing/CounterGroup.cs | 59 ++++++++++++++----- 1 file changed, 43 insertions(+), 16 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs index 71af1ba5b2097..a83e30381b7cf 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs @@ -155,7 +155,7 @@ private void EnableTimer(float pollingIntervalInSeconds) if (_pollingIntervalInMilliseconds == 0 || pollingIntervalInSeconds * 1000 < _pollingIntervalInMilliseconds) { _pollingIntervalInMilliseconds = (int)(pollingIntervalInSeconds * 1000); - ResetCounters(); // Reset statistics for counters before we start the thread. + HandleCountersReset(); // Schedule IncrementingPollingCounter reset and synchronously reset other counters before we start the thread _timeStampSinceCollectionStarted = DateTime.UtcNow; _nextPollingTimeStamp = DateTime.UtcNow + new TimeSpan(0, 0, (int)pollingIntervalInSeconds); @@ -189,26 +189,35 @@ private void DisableTimer() Debug.Assert(Monitor.IsEntered(s_counterGroupLock)); _pollingIntervalInMilliseconds = 0; s_counterGroupEnabledList?.Remove(this); + + if (s_needsResetIncrementingPollingCounters.Count > 0) + { + foreach (DiagnosticCounter diagnosticCounter in _counters) + { + if (diagnosticCounter is IncrementingPollingCounter pollingCounter) + s_needsResetIncrementingPollingCounters.Remove(pollingCounter); + } + } } - private void ResetCounters() + private void HandleCountersReset() { - lock (s_counterGroupLock) // Lock the CounterGroup + Debug.Assert(Monitor.IsEntered(s_counterGroupLock)); + foreach (DiagnosticCounter counter in _counters) { - foreach (DiagnosticCounter counter in _counters) + if (counter is IncrementingEventCounter ieCounter) { - if (counter is IncrementingEventCounter ieCounter) - { - ieCounter.UpdateMetric(); - } - else if (counter is IncrementingPollingCounter ipCounter) - { - ipCounter.UpdateMetric(); - } - else if (counter is EventCounter eCounter) - { - eCounter.ResetStatistics(); - } + ieCounter.UpdateMetric(); + } + else if (counter is IncrementingPollingCounter ipCounter) + { + // IncrementingPollingCounters will be reset on timer thread + // We need this to avoid deadlocks caused by running IncrementingPollingCounter._totalValueProvider under EventListener.EventListenersLock + s_needsResetIncrementingPollingCounters.Add(ipCounter); + } + else if (counter is EventCounter eCounter) + { + eCounter.ResetStatistics(); } } } @@ -264,6 +273,7 @@ private void OnTimer() private static AutoResetEvent? s_pollingThreadSleepEvent; private static List? s_counterGroupEnabledList; + private static readonly List s_needsResetIncrementingPollingCounters = []; private static void PollForValues() { @@ -274,6 +284,7 @@ private static void PollForValues() // calling into the callbacks can cause a re-entrancy into CounterGroup.Enable() // and result in a deadlock. (See https://github.com/dotnet/runtime/issues/40190 for details) var onTimers = new List(); + List countersToReset = []; while (true) { int sleepDurationInMilliseconds = int.MaxValue; @@ -292,7 +303,23 @@ private static void PollForValues() millisecondsTillNextPoll = Math.Max(1, millisecondsTillNextPoll); sleepDurationInMilliseconds = Math.Min(sleepDurationInMilliseconds, millisecondsTillNextPoll); } + + if (s_needsResetIncrementingPollingCounters.Count > 0) + { + foreach (IncrementingPollingCounter counter in s_needsResetIncrementingPollingCounters) + { + countersToReset.Add(counter); + } + s_needsResetIncrementingPollingCounters.Clear(); + } + } + + foreach (IncrementingPollingCounter counter in countersToReset) + { + counter.UpdateMetric(); } + countersToReset.Clear(); + foreach (CounterGroup onTimer in onTimers) { onTimer.OnTimer(); From f459a4827e0d9db99192bdb95aadbf7da390d463 Mon Sep 17 00:00:00 2001 From: eterekhin Date: Sat, 3 Aug 2024 14:44:54 +0200 Subject: [PATCH 2/3] Made the comment clearer Co-authored-by: Noah Falk --- .../src/System/Diagnostics/Tracing/CounterGroup.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs index a83e30381b7cf..044fd1b90fabc 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs @@ -155,7 +155,8 @@ private void EnableTimer(float pollingIntervalInSeconds) if (_pollingIntervalInMilliseconds == 0 || pollingIntervalInSeconds * 1000 < _pollingIntervalInMilliseconds) { _pollingIntervalInMilliseconds = (int)(pollingIntervalInSeconds * 1000); - HandleCountersReset(); // Schedule IncrementingPollingCounter reset and synchronously reset other counters before we start the thread + // Schedule IncrementingPollingCounter reset and synchronously reset other counters + HandleCountersReset(); _timeStampSinceCollectionStarted = DateTime.UtcNow; _nextPollingTimeStamp = DateTime.UtcNow + new TimeSpan(0, 0, (int)pollingIntervalInSeconds); From 2fa1554487a5165afd28db2ae5358b176aa4d03c Mon Sep 17 00:00:00 2001 From: "Evgeny.Terekhin" Date: Sat, 3 Aug 2024 14:56:10 +0200 Subject: [PATCH 3/3] Simplified counters reset code --- .../Diagnostics/Tracing/CounterGroup.cs | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs index 044fd1b90fabc..53811bda9eb44 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs @@ -156,7 +156,7 @@ private void EnableTimer(float pollingIntervalInSeconds) { _pollingIntervalInMilliseconds = (int)(pollingIntervalInSeconds * 1000); // Schedule IncrementingPollingCounter reset and synchronously reset other counters - HandleCountersReset(); + HandleCountersReset(); _timeStampSinceCollectionStarted = DateTime.UtcNow; _nextPollingTimeStamp = DateTime.UtcNow + new TimeSpan(0, 0, (int)pollingIntervalInSeconds); @@ -274,7 +274,7 @@ private void OnTimer() private static AutoResetEvent? s_pollingThreadSleepEvent; private static List? s_counterGroupEnabledList; - private static readonly List s_needsResetIncrementingPollingCounters = []; + private static List s_needsResetIncrementingPollingCounters = []; private static void PollForValues() { @@ -285,7 +285,7 @@ private static void PollForValues() // calling into the callbacks can cause a re-entrancy into CounterGroup.Enable() // and result in a deadlock. (See https://github.com/dotnet/runtime/issues/40190 for details) var onTimers = new List(); - List countersToReset = []; + List? countersToReset = null; while (true) { int sleepDurationInMilliseconds = int.MaxValue; @@ -307,19 +307,20 @@ private static void PollForValues() if (s_needsResetIncrementingPollingCounters.Count > 0) { - foreach (IncrementingPollingCounter counter in s_needsResetIncrementingPollingCounters) - { - countersToReset.Add(counter); - } - s_needsResetIncrementingPollingCounters.Clear(); + countersToReset = s_needsResetIncrementingPollingCounters; + s_needsResetIncrementingPollingCounters = []; } } - foreach (IncrementingPollingCounter counter in countersToReset) + if (countersToReset != null) { - counter.UpdateMetric(); + foreach (IncrementingPollingCounter counter in countersToReset) + { + counter.UpdateMetric(); + } + + countersToReset = null; } - countersToReset.Clear(); foreach (CounterGroup onTimer in onTimers) {