From 1ead49f843fdc963c1c64052e7f125882e033576 Mon Sep 17 00:00:00 2001 From: David Mason Date: Fri, 7 Apr 2023 17:25:01 -0700 Subject: [PATCH 1/3] Port 82970 to 6.0 --- .../Diagnostics/Tracing/CounterGroup.cs | 61 ++++++++++++++----- .../System/Diagnostics/Tracing/EventSource.cs | 6 +- 2 files changed, 48 insertions(+), 19 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 aec84922437a36..2b1f6f4b4242cd 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 @@ -52,24 +52,56 @@ private void RegisterCommandCallback() private void OnEventSourceCommand(object? sender, EventCommandEventArgs e) { - if (e.Command == EventCommand.Enable || e.Command == EventCommand.Update) - { - Debug.Assert(e.Arguments != null); + // Should only be enable or disable + Debug.Assert(e.Command == EventCommand.Enable || e.Command == EventCommand.Disable); - if (e.Arguments.TryGetValue("EventCounterIntervalSec", out string? valueStr) && float.TryParse(valueStr, out float value)) + lock (s_counterGroupLock) // Lock the CounterGroup + { + if (e.Command == EventCommand.Enable) { - lock (s_counterGroupLock) // Lock the CounterGroup + Debug.Assert(e.Arguments != null); + + if (!e.Arguments.TryGetValue("EventCounterIntervalSec", out string? valueStr) + || !float.TryParse(valueStr, out float intervalValue)) + { + // Command is Enable but no EventCounterIntervalSec arg so ignore + return; + } + + // Sending an Enabled with EventCounterIntervalSec <=0 is a signal that we should immediately turn + // off counters + if (intervalValue <= 0) + { + DisableTimer(); + } + else { - EnableTimer(value); + EnableTimer(intervalValue); } } - } - else if (e.Command == EventCommand.Disable) - { - lock (s_counterGroupLock) + else { - DisableTimer(); + Debug.Assert(e.Command == EventCommand.Disable); + // Since we allow sessions to send multiple Enable commands to update the interval, we cannot + // rely on ref counting to determine when to enable and disable counters. You will get an arbitrary + // number of Enables and one Disable per session. + // + // Previously we would turn off counters when we received any Disable command, but that meant that any + // session could turn off counters for all other sessions. To get to a good place we now will only + // turn off counters once the EventSource that provides the counters is disabled. We can then end up + // keeping counters on too long in certain circumstances - if one session enables counters, then a second + // session enables the EventSource but not counters we will stay on until both sessions terminate, even + // if the first session terminates first. + if (!_eventSource.IsEnabled()) + { + DisableTimer(); + } } + + Debug.Assert((s_counterGroupEnabledList == null && !_eventSource.IsEnabled()) + || (_eventSource.IsEnabled() && s_counterGroupEnabledList!.Contains(this)) + || (_pollingIntervalInMilliseconds == 0 && !s_counterGroupEnabledList!.Contains(this)) + || (!_eventSource.IsEnabled() && !s_counterGroupEnabledList!.Contains(this))); } } @@ -124,12 +156,9 @@ internal static CounterGroup GetCounterGroup(EventSource eventSource) private void EnableTimer(float pollingIntervalInSeconds) { + Debug.Assert(pollingIntervalInSeconds > 0); Debug.Assert(Monitor.IsEntered(s_counterGroupLock)); - if (pollingIntervalInSeconds <= 0) - { - DisableTimer(); - } - else if (_pollingIntervalInMilliseconds == 0 || pollingIntervalInSeconds * 1000 < _pollingIntervalInMilliseconds) + if (_pollingIntervalInMilliseconds == 0 || pollingIntervalInSeconds * 1000 < _pollingIntervalInMilliseconds) { _pollingIntervalInMilliseconds = (int)(pollingIntervalInSeconds * 1000); ResetCounters(); // Reset statistics for counters before we start the thread. diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs index 80c26cc0319b44..f5fe93b02a0aba 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs @@ -2733,9 +2733,6 @@ internal void DoCommand(EventCommandEventArgs commandArgs) m_eventSourceEnabled = true; } - this.OnEventCommand(commandArgs); - this.m_eventCommandExecuted?.Invoke(this, commandArgs); - if (!commandArgs.enable) { // If we are disabling, maybe we can turn on 'quick checks' to filter @@ -2767,6 +2764,9 @@ internal void DoCommand(EventCommandEventArgs commandArgs) m_eventSourceEnabled = false; } } + + this.OnEventCommand(commandArgs); + this.m_eventCommandExecuted?.Invoke(this, commandArgs); } else { From fb6a4af99452f15f7fb620e106f613e97740883f Mon Sep 17 00:00:00 2001 From: David Mason Date: Fri, 14 Apr 2023 15:38:09 -0700 Subject: [PATCH 2/3] Code Review feedback --- .../Diagnostics/Tracing/CounterGroup.cs | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 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 2b1f6f4b4242cd..455bf4f438e7f6 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 @@ -57,7 +57,7 @@ private void OnEventSourceCommand(object? sender, EventCommandEventArgs e) lock (s_counterGroupLock) // Lock the CounterGroup { - if (e.Command == EventCommand.Enable) + if (e.Command == EventCommand.Enable || e.Command == EventCommand.Update) { Debug.Assert(e.Arguments != null); @@ -68,18 +68,9 @@ private void OnEventSourceCommand(object? sender, EventCommandEventArgs e) return; } - // Sending an Enabled with EventCounterIntervalSec <=0 is a signal that we should immediately turn - // off counters - if (intervalValue <= 0) - { - DisableTimer(); - } - else - { - EnableTimer(intervalValue); - } + EnableTimer(intervalValue); } - else + else if (e.Command == EventCommand.Disable) { Debug.Assert(e.Command == EventCommand.Disable); // Since we allow sessions to send multiple Enable commands to update the interval, we cannot @@ -158,7 +149,11 @@ private void EnableTimer(float pollingIntervalInSeconds) { Debug.Assert(pollingIntervalInSeconds > 0); Debug.Assert(Monitor.IsEntered(s_counterGroupLock)); - if (_pollingIntervalInMilliseconds == 0 || pollingIntervalInSeconds * 1000 < _pollingIntervalInMilliseconds) + if (pollingIntervalInSeconds <= 0) + { + DisableTimer(); + } + else if (_pollingIntervalInMilliseconds == 0 || pollingIntervalInSeconds * 1000 < _pollingIntervalInMilliseconds) { _pollingIntervalInMilliseconds = (int)(pollingIntervalInSeconds * 1000); ResetCounters(); // Reset statistics for counters before we start the thread. From 4a618fd9106997d72af638c09ecdf75b056aade7 Mon Sep 17 00:00:00 2001 From: David Mason Date: Wed, 10 May 2023 00:48:22 -0700 Subject: [PATCH 3/3] Make asserts match code after code review feedback --- .../src/System/Diagnostics/Tracing/CounterGroup.cs | 2 -- 1 file changed, 2 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 455bf4f438e7f6..77c29cf159801d 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 @@ -72,7 +72,6 @@ private void OnEventSourceCommand(object? sender, EventCommandEventArgs e) } else if (e.Command == EventCommand.Disable) { - Debug.Assert(e.Command == EventCommand.Disable); // Since we allow sessions to send multiple Enable commands to update the interval, we cannot // rely on ref counting to determine when to enable and disable counters. You will get an arbitrary // number of Enables and one Disable per session. @@ -147,7 +146,6 @@ internal static CounterGroup GetCounterGroup(EventSource eventSource) private void EnableTimer(float pollingIntervalInSeconds) { - Debug.Assert(pollingIntervalInSeconds > 0); Debug.Assert(Monitor.IsEntered(s_counterGroupLock)); if (pollingIntervalInSeconds <= 0) {