From daeb1284acde4665720bd97a9e89be4d40d5cbb2 Mon Sep 17 00:00:00 2001 From: Noah Falk Date: Thu, 8 Aug 2024 11:05:48 -0700 Subject: [PATCH] PR feedback --- .../ILLink.Descriptors.LibraryBuild.xml | 2 +- ...System.Diagnostics.DiagnosticSource.csproj | 2 +- .../Diagnostics/Metrics/MetricsEventSource.cs | 4 +-- .../tests/MetricEventSourceTests.cs | 4 +-- .../System/Diagnostics/Tracing/EventSource.cs | 26 ++++++++++--------- 5 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/ILLink/ILLink.Descriptors.LibraryBuild.xml b/src/libraries/System.Diagnostics.DiagnosticSource/src/ILLink/ILLink.Descriptors.LibraryBuild.xml index 73a3fd592c6d5..a42d7f0e7e82d 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/ILLink/ILLink.Descriptors.LibraryBuild.xml +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/ILLink/ILLink.Descriptors.LibraryBuild.xml @@ -1,7 +1,7 @@  - + diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj index 78200cbe8ab83..3db1dbd5a06ef 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj @@ -25,6 +25,7 @@ System.Diagnostics.DiagnosticSource + @@ -85,7 +86,6 @@ System.Diagnostics.DiagnosticSource - diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs index 325ef4e3bca1f..520307cc4c479 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs @@ -47,8 +47,8 @@ internal sealed class MetricsEventSource : EventSource public static readonly MetricsEventSource Log = new(); // Although this API isn't public, it is invoked via reflection from System.Private.CoreLib and needs the same back-compat - // consideration as a public API. See EventSource.InitializeDefaultEventSources() in S.P.C source for more details. - // We have a unit test GetInstanceMethodIsReflectable that verifies this method isn't accidentally removed or renamed. + // consideration as a public API. See EventSource.InitializeDefaultEventSources() in System.Private.CoreLib source for more + // details. We have a unit test GetInstanceMethodIsReflectable that verifies this method isn't accidentally removed or renamed. public static MetricsEventSource GetInstance() { return Log; } private const string SharedSessionId = "SHARED"; diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricEventSourceTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricEventSourceTests.cs index e0c8a68b05214..b43aba298f3f0 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricEventSourceTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricEventSourceTests.cs @@ -30,8 +30,8 @@ public MetricEventSourceTests(ITestOutputHelper output) [Fact] public void GetInstanceMethodIsReflectable() { - // The startup code in S.P.C needs to be able to get the MetricsEventSource instance via reflection. See EventSource.InitializeDefaultEventSources() in - // the S.P.C source. + // The startup code in System.Private.CoreLib needs to be able to get the MetricsEventSource instance via reflection. See EventSource.InitializeDefaultEventSources() in + // the System.Private.CoreLib source. // Even though the the type isn't public this test ensures the GetInstance() API isn't removed or renamed. Type? metricsEventSourceType = Type.GetType("System.Diagnostics.Metrics.MetricsEventSource, System.Diagnostics.DiagnosticSource", throwOnError: false); Assert.True(metricsEventSourceType != null, "Unable to get MetricsEventSource type via reflection"); 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 67d4857e599da..3edb4ad9d54f6 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 @@ -2407,19 +2407,19 @@ private sealed class OverrideEventProvider : EventProvider public OverrideEventProvider(Func eventSourceFactory, EventProviderType providerType) : base(providerType) { - this.m_eventSourceFactory = eventSourceFactory; - this.m_eventProviderType = providerType; + _eventSourceFactory = eventSourceFactory; + _eventProviderType = providerType; } internal override void OnControllerCommand(ControllerCommand command, IDictionary? arguments, int perEventSourceSessionId) { // We use null to represent the ETW EventListener. EventListener? listener = null; - m_eventSourceFactory()?.SendCommand(listener, m_eventProviderType, perEventSourceSessionId, + _eventSourceFactory()?.SendCommand(listener, _eventProviderType, perEventSourceSessionId, (EventCommand)command, IsEnabled(), Level, MatchAnyKeyword, arguments); } - private readonly Func m_eventSourceFactory; - private readonly EventProviderType m_eventProviderType; + private readonly Func _eventSourceFactory; + private readonly EventProviderType _eventProviderType; } /// @@ -3833,7 +3833,7 @@ internal static void InitializeDefaultEventSources() // pulling the System.Diagnostics.DiagnosticSource assembly into the process until it is needed. if (AppContext.TryGetSwitch("System.Diagnostics.Metrics.Meter.IsSupported", out bool isSupported) ? isSupported : true) { - string name = "System.Diagnostics.Metrics"; + const string name = "System.Diagnostics.Metrics"; Guid id = new Guid("20752bc4-c151-50f5-f27b-df92d8af5a61"); PreregisterEventProviders(id, name, GetMetricsEventSource); } @@ -3869,6 +3869,10 @@ private static unsafe void PreregisterEventProviders(Guid id, string name, Func< // traits but the SetInformation() call we use below generates metadata from name only. If we want traits support // in the future it could be added. + // NOTE: You might think this preregister logic could be simplified by using an Action to create the EventSource instead of + // Func and then allow the EventSource to initialize as normal. This doesn't work however because calling + // EtwEventProvider.Register() inside of an ETW callback deadlocks. Instead we have to bind the EventSource to the + // EtwEventProvider that was already registered and use the callback we got on that provider to invoke EventSource.SendCommand(). try { s_preregisteredEventSourceFactories.Add(eventSourceFactory); @@ -3936,26 +3940,24 @@ internal static void EnsurePreregisteredEventSourcesExist() { lock (s_preregisteredEtwProviders) { - s_preregisteredEtwProviders.TryGetValue(id, out OverrideEventProvider? provider); - s_preregisteredEtwProviders.Remove(id); + s_preregisteredEtwProviders.Remove(id, out OverrideEventProvider? provider); return provider; } } - private static Dictionary s_preregisteredEtwProviders = new Dictionary(); + private static readonly Dictionary s_preregisteredEtwProviders = new Dictionary(); #if FEATURE_PERFTRACING private static OverrideEventProvider? TryGetPreregisteredEventPipeProvider(string name) { lock (s_preregisteredEventPipeProviders) { - s_preregisteredEventPipeProviders.TryGetValue(name, out OverrideEventProvider? provider); - s_preregisteredEventPipeProviders.Remove(name); + s_preregisteredEventPipeProviders.Remove(name, out OverrideEventProvider? provider); return provider; } } - private static Dictionary s_preregisteredEventPipeProviders = new Dictionary(); + private static readonly Dictionary s_preregisteredEventPipeProviders = new Dictionary(); #endif // private instance state