Enable starting SampleProfiler from startup EventPipe session (#33200)

* Enable SampleProfiler to be enabled from startup

* Remove EventPipeController.cs

* PR feedback

9 files changed, 61 insertions, 267 deletions

diff --git a/src/coreclr/clrfeatures.cmake b/src/coreclr/clrfeatures.cmake
index 4a1f2b7225d..078b4e73ac8 100644
--- a/src/coreclr/clrfeatures.cmake
+++ b/src/coreclr/clrfeatures.cmake
@@ -27,7 +27,3 @@ endif(NOT DEFINED FEATURE_STANDALONE_GC)
 if(NOT DEFINED FEATURE_AUTO_TRACE)
   set(FEATURE_AUTO_TRACE 0)
 endif(NOT DEFINED FEATURE_AUTO_TRACE)
-
-if(NOT DEFINED FEATURE_EVENTPIPE_STARTUP)
-  set(FEATURE_EVENTPIPE_STARTUP 0)
-endif(NOT DEFINED FEATURE_EVENTPIPE_STARTUP)
diff --git a/src/coreclr/src/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/coreclr/src/System.Private.CoreLib/System.Private.CoreLib.csproj
index 653e61bf4d8..881d4601cd6 100644
--- a/src/coreclr/src/System.Private.CoreLib/System.Private.CoreLib.csproj
+++ b/src/coreclr/src/System.Private.CoreLib/System.Private.CoreLib.csproj
@@ -152,7 +152,6 @@
     <Compile Include="$(BclSourcesRoot)\System\Diagnostics\Debugger.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Diagnostics\EditAndContinueHelper.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Diagnostics\Eventing\EventPipe.cs" />
-    <Compile Include="$(BclSourcesRoot)\System\Diagnostics\Eventing\EventPipeController.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Diagnostics\Eventing\EventPipeEventDispatcher.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Diagnostics\Eventing\EventPipeEventProvider.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Diagnostics\Eventing\EventPipeMetadataGenerator.cs" />
diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipeController.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipeController.cs
deleted file mode 100644
index e38b6dbe32a..00000000000
--- a/src/coreclr/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipeController.cs
+++ /dev/null
@@ -1,221 +0,0 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
-#if FEATURE_PERFTRACING
-using System.IO;
-using System.Reflection;
-using System.Runtime.Versioning;
-
-namespace System.Diagnostics.Tracing
-{
-    /// <summary>
-    /// Simple out-of-process listener for controlling EventPipe.
-    /// The following environment variables are used to configure EventPipe:
-    ///  - COMPlus_EnableEventPipe=1 : Enable EventPipe immediately for the life of the process.
-    ///  - COMPlus_EventPipeConfig : Provides the configuration in xperf string form for which providers/keywords/levels to be enabled.
-    ///                              If not specified, the default configuration is used.
-    ///  - COMPlus_EventPipeOutputFile : The full path to the netperf file to be written.
-    ///  - COMPlus_EventPipeCircularMB : The size in megabytes of the circular buffer.
-    /// </summary>
-    internal static class EventPipeController
-    {
-        // Miscellaneous constants.
-        private const string DefaultAppName = "app";
-        private const string NetPerfFileExtension = ".netperf";
-        private const string NetTraceFileExtension = ".nettrace";
-        private const uint DefaultCircularBufferMB = 256; // MB (PerfView and dotnet-trace default)
-        private const char ProviderConfigDelimiter = ',';
-        private const char ConfigComponentDelimiter = ':';
-
-        // The default set of providers/keywords/levels.  Used if an alternative configuration is not specified.
-        private static EventPipeProviderConfiguration[] DefaultProviderConfiguration => new EventPipeProviderConfiguration[]
-        {
-            new EventPipeProviderConfiguration("Microsoft-Windows-DotNETRuntime", 0x4c14fccbd, 5, null),
-            new EventPipeProviderConfiguration("Microsoft-Windows-DotNETRuntimePrivate", 0x4002000b, 5, null),
-            new EventPipeProviderConfiguration("Microsoft-DotNETCore-SampleProfiler", 0x0, 5, null),
-        };
-
-        private static bool IsControllerInitialized { get; set; } = false;
-
-        internal static void Initialize()
-        {
-            // Don't allow failures to propagate upstream.  Ensure program correctness without tracing.
-            try
-            {
-                if (!IsControllerInitialized)
-                {
-                    if (Config_EnableEventPipe > 0)
-                    {
-                        // Enable tracing immediately.
-                        // It will be disabled automatically on shutdown.
-                        EventPipe.Enable(BuildConfigFromEnvironment());
-                    }
-
-                    RuntimeEventSource.Initialize();
-
-                    IsControllerInitialized = true;
-                }
-            }
-            catch { }
-        }
-
-        private static EventPipeConfiguration BuildConfigFromEnvironment()
-        {
-            // Build the full path to the trace file.
-            string traceFileName = BuildTraceFileName();
-            string outputFilePath = Path.Combine(Config_EventPipeOutputPath, traceFileName);
-
-            // Create a new configuration object.
-            EventPipeConfiguration config = new EventPipeConfiguration(
-                outputFilePath,
-                (Config_NetTraceFormat != 0) ? EventPipeSerializationFormat.NetTrace : EventPipeSerializationFormat.NetPerf,
-                Config_EventPipeCircularMB);
-
-            // Get the configuration.
-            string? strConfig = Config_EventPipeConfig;
-            if (!string.IsNullOrEmpty(strConfig))
-            {
-                // If the configuration is specified, parse it and save it to the config object.
-                SetProviderConfiguration(strConfig, config);
-            }
-            else
-            {
-                // Specify the default configuration.
-                config.EnableProviderRange(DefaultProviderConfiguration);
-            }
-
-            return config;
-        }
-
-        private static string BuildTraceFileName()
-        {
-            return GetAppName() + "." + Interop.GetCurrentProcessId().ToString() +
-              ((Config_NetTraceFormat != 0) ? NetTraceFileExtension : NetPerfFileExtension);
-        }
-
-        private static string GetAppName()
-        {
-            string? appName = null;
-            Assembly? entryAssembly = Assembly.GetEntryAssembly();
-            if (entryAssembly != null)
-            {
-                AssemblyName? assemblyName = entryAssembly.GetName();
-                if (assemblyName != null)
-                {
-                    appName = assemblyName.Name;
-                }
-            }
-
-            if (string.IsNullOrEmpty(appName))
-            {
-                appName = DefaultAppName;
-            }
-
-            return appName;
-        }
-
-        private static void SetProviderConfiguration(string strConfig, EventPipeConfiguration config)
-        {
-            if (string.IsNullOrEmpty(strConfig))
-            {
-                throw new ArgumentNullException(nameof(strConfig));
-            }
-
-            // Provider format: "(GUID|KnownProviderName)[:Flags[:Level][:KeyValueArgs]]"
-            // where KeyValueArgs are of the form: "[key1=value1][;key2=value2]"
-            // `strConfig` must be of the form "Provider[,Provider]"
-            string[] providers = strConfig.Split(
-                ProviderConfigDelimiter,
-                StringSplitOptions.RemoveEmptyEntries); // Remove "empty" providers.
-            foreach (string provider in providers)
-            {
-                // Split expecting a maximum of four tokens.
-                string[] components = provider.Split(
-                    ConfigComponentDelimiter,
-                    4, // if there is ':' in the parameters then anything after it will not be ignored.
-                    StringSplitOptions.None); // Keep empty tokens
-
-                string? providerName = components.Length > 0 ? components[0] : null;
-                if (string.IsNullOrEmpty(providerName))
-                    continue;  // No provider name specified.
-
-                ulong keywords = ulong.MaxValue;
-                if (components.Length > 1)
-                {
-                    // We use a try/catch block here because ulong.TryParse won't accept 0x at the beginning
-                    // of a hex string.  Thus, we either need to conditionally strip it or handle the exception.
-                    // Given that this is not a perf-critical path, catching the exception is the simpler code.
-                    try
-                    {
-                        keywords = Convert.ToUInt64(components[1], 16);
-                    }
-                    catch
-                    {
-                    }
-                }
-
-                uint level = 5; // Verbose
-                if (components.Length > 2)
-                {
-                    uint.TryParse(components[2], out level);
-                }
-
-                string? filterData = components.Length > 3 ? components[3] : null;
-
-                config.EnableProviderWithFilter(providerName, keywords, level, filterData);
-            }
-        }
-
-        /// <summary>
-        /// Returns -1 if the EnableEventPipe environment variable is not set at all (or is illegal)
-        /// </summary>
-        private static int Config_EnableEventPipe
-        {
-            get
-            {
-                string? stringValue = CompatibilitySwitch.GetValueInternal("EnableEventPipe");
-                if ((stringValue == null) || (!int.TryParse(stringValue, out int value)))
-                {
-                    value = -1;     // Indicates no value (or is illegal)
-                }
-
-                return value;
-            }
-        }
-
-        private static int Config_NetTraceFormat
-        {
-            get
-            {
-                string? stringValue = CompatibilitySwitch.GetValueInternal("EventPipeNetTraceFormat");
-                if ((stringValue == null) || (!int.TryParse(stringValue, out int value)))
-                {
-                    value = -1;     // Indicates no value (or is illegal)
-                }
-
-                return value;
-            }
-        }
-
-        private static string? Config_EventPipeConfig => CompatibilitySwitch.GetValueInternal("EventPipeConfig");
-
-        private static uint Config_EventPipeCircularMB
-        {
-            get
-            {
-                string? stringValue = CompatibilitySwitch.GetValueInternal("EventPipeCircularMB");
-                if ((stringValue == null) || (!uint.TryParse(stringValue, out uint value)))
-                {
-                    value = DefaultCircularBufferMB;
-                }
-
-                return value;
-            }
-        }
-
-        private static string Config_EventPipeOutputPath =>
-            CompatibilitySwitch.GetValueInternal("EventPipeOutputPath") ?? ".";
-    }
-}
-
-#endif // FEATURE_PERFTRACING
diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs b/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs
index 220c7bac4bb..cfe7daca7ad 100644
--- a/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs
+++ b/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs
@@ -26,7 +26,7 @@ namespace System
         private static void ProcessStartupHooks()
         {
             // Initialize tracing before any user code can be called.
-            System.Diagnostics.Tracing.EventPipeController.Initialize();
+            System.Diagnostics.Tracing.RuntimeEventSource.Initialize();
 
             string? startupHooksVariable = (string?)AppContext.GetData("STARTUP_HOOKS");
             if (startupHooksVariable == null)
diff --git a/src/coreclr/src/vm/ceemain.cpp b/src/coreclr/src/vm/ceemain.cpp
index 3f500009422..2790349ee93 100644
--- a/src/coreclr/src/vm/ceemain.cpp
+++ b/src/coreclr/src/vm/ceemain.cpp
@@ -840,9 +840,7 @@ void EEStartupHelper(COINITIEE fFlags)
 #ifndef CROSSGEN_COMPILE
 
         InitializeGarbageCollector();
-
-        // Initialize remoting
-
+        
         if (!GCHandleUtilities::GetGCHandleManager()->Initialize())
         {
             IfFailGo(E_OUTOFMEMORY);
@@ -947,6 +945,11 @@ void EEStartupHelper(COINITIEE fFlags)
         hr = g_pGCHeap->Initialize();
         IfFailGo(hr);
 
+        // Finish setting up rest of EventPipe - specifically enable SampleProfiler if it was requested at startup.
+        // SampleProfiler needs to cooperate with the GC which hasn't fully finished setting up in the first part of the
+        // EventPipe initialization, so this is done after the GC has been fully initialized.
+        EventPipe::FinishInitialize();
+
         // This isn't done as part of InitializeGarbageCollector() above because thread
         // creation requires AppDomains to have been set up.
         FinalizerThread::FinalizerThreadCreate();
diff --git a/src/coreclr/src/vm/eventpipe.cpp b/src/coreclr/src/vm/eventpipe.cpp
index 06d0ed7ac2c..a7b64acc216 100644
--- a/src/coreclr/src/vm/eventpipe.cpp
+++ b/src/coreclr/src/vm/eventpipe.cpp
@@ -39,6 +39,7 @@ Volatile<uint64_t> EventPipe::s_allowWrite = 0;
 unsigned int * EventPipe::s_pProcGroupOffsets = nullptr;
 #endif
 Volatile<uint32_t> EventPipe::s_numberOfSessions(0);
+bool EventPipe::s_enableSampleProfilerAtStartup = false;
 
 // This function is auto-generated from /src/scripts/genEventPipe.py
 #ifdef TARGET_UNIX
@@ -100,9 +101,18 @@ void EventPipe::Initialize()
         if (tracingInitialized)
             s_state = EventPipeState::Initialized;
     }
-#ifdef FEATURE_EVENTPIPE_STARTUP
     EnableViaEnvironmentVariables();
-#endif // FEATURE_EVENTPIPE_STARTUP
+}
+
+// Finish setting up the rest of EventPipe.
+void EventPipe::FinishInitialize()
+{
+    STANDARD_VM_CONTRACT;
+
+    if (s_enableSampleProfilerAtStartup)
+    {
+        SampleProfiler::Enable();
+    }
 }
 
 //
@@ -137,10 +147,12 @@ void EventPipe::EnableViaEnvironmentVariables()
         // with the default provider configurations.
         if (configToParse == nullptr || *configToParse == L'\0')
         {
-            providerCnt = 2;
+            providerCnt = 3;
             pProviders = new EventPipeProviderConfiguration[providerCnt];
             pProviders[0] = EventPipeProviderConfiguration(W("Microsoft-Windows-DotNETRuntime"), 0x4c14fccbd, 5, nullptr);
             pProviders[1] = EventPipeProviderConfiguration(W("Microsoft-Windows-DotNETRuntimePrivate"), 0x4002000b, 5, nullptr);
+            pProviders[2] = EventPipeProviderConfiguration(W("Microsoft-DotNETCore-SampleProfiler"), 0x0, 5, nullptr);
+            s_enableSampleProfilerAtStartup = true;
         }
         else
         {
@@ -169,21 +181,19 @@ void EventPipe::EnableViaEnvironmentVariables()
                 {
                     return;
                 }
-                // SampleProfiler can't be enabled on startup yet.
-                else if (wcscmp(W("Microsoft-DotNETCore-SampleProfiler"), configuration.GetProviderName()) == 0)
-                {
-                    providerCnt -= 1;
-                }
-                else
+
+                if (wcscmp(W("Microsoft-DotNETCore-SampleProfiler"), configuration.GetProviderName()) == 0)
                 {
-                    pProviders[i++] = EventPipeProviderConfiguration(
-                        configuration.GetProviderName(),
-                        configuration.GetEnabledKeywordsMask(),
-                        configuration.GetLevel(),
-                        configuration.GetArgument()
-                    );
+                    s_enableSampleProfilerAtStartup = true;
                 }
 
+                pProviders[i++] = EventPipeProviderConfiguration(
+                    configuration.GetProviderName(),
+                    configuration.GetEnabledKeywordsMask(),
+                    configuration.GetLevel(),
+                    configuration.GetArgument()
+                );
+
                 if (end == nullptr)
                 {
                     break;
@@ -192,20 +202,18 @@ void EventPipe::EnableViaEnvironmentVariables()
             }
         }
 
-        if (providerCnt != 0)
-        {
-            uint64_t sessionID = EventPipe::Enable(
-                outputPath,
-                eventpipeCircularBufferMB,
-                pProviders,
-                providerCnt,
-                EventPipeSessionType::File,
-                EventPipeSerializationFormat::NetTraceV4,
-                true,
-                nullptr
-            );
-            EventPipe::StartStreaming(sessionID);
-        }
+        uint64_t sessionID = EventPipe::Enable(
+            outputPath,
+            eventpipeCircularBufferMB,
+            pProviders,
+            providerCnt,
+            EventPipeSessionType::File,
+            EventPipeSerializationFormat::NetTraceV4,
+            true,
+            nullptr,
+            false
+        );
+        EventPipe::StartStreaming(sessionID);
     }
 }
 
@@ -274,7 +282,8 @@ EventPipeSessionID EventPipe::Enable(
     EventPipeSessionType sessionType,
     EventPipeSerializationFormat format,
     const bool rundownRequested,
-    IpcStream *const pStream)
+    IpcStream *const pStream,
+    const bool enableSampleProfiler)
 {
     CONTRACTL
     {
@@ -313,7 +322,7 @@ EventPipeSessionID EventPipe::Enable(
             pProviders,
             numProviders);
 
-        const bool fSuccess = EnableInternal(pSession, pEventPipeProviderCallbackDataQueue);
+        const bool fSuccess = EnableInternal(pSession, pEventPipeProviderCallbackDataQueue, enableSampleProfiler);
         if (fSuccess)
             sessionId = reinterpret_cast<EventPipeSessionID>(pSession);
         else
@@ -325,7 +334,8 @@ EventPipeSessionID EventPipe::Enable(
 
 bool EventPipe::EnableInternal(
     EventPipeSession *const pSession,
-    EventPipeProviderCallbackDataQueue* pEventPipeProviderCallbackDataQueue)
+    EventPipeProviderCallbackDataQueue* pEventPipeProviderCallbackDataQueue,
+    const bool enableSampleProfiler)
 {
     CONTRACTL
     {
@@ -374,7 +384,10 @@ bool EventPipe::EnableInternal(
     s_config.Enable(*pSession, pEventPipeProviderCallbackDataQueue);
 
     // Enable the sample profiler
-    SampleProfiler::Enable(pEventPipeProviderCallbackDataQueue);
+    if (enableSampleProfiler)
+    {
+        SampleProfiler::Enable();
+    }
 
     return true;
 }
diff --git a/src/coreclr/src/vm/eventpipe.h b/src/coreclr/src/vm/eventpipe.h
index a638ca118cd..2217b49776a 100644
--- a/src/coreclr/src/vm/eventpipe.h
+++ b/src/coreclr/src/vm/eventpipe.h
@@ -46,6 +46,7 @@ public:
 
     // Initialize the event pipe.
     static void Initialize();
+    static void FinishInitialize();
 
     // Initialize environment variable based session
     static void EnableViaEnvironmentVariables();
@@ -62,7 +63,8 @@ public:
         EventPipeSessionType sessionType,
         EventPipeSerializationFormat format,
         const bool rundownRequested,
-        IpcStream *const pStream);
+        IpcStream *const pStream,
+        const bool enableSampleProfiler = true);
 
     // Disable tracing via the event pipe.
     static void Disable(EventPipeSessionID id);
@@ -178,7 +180,8 @@ private:
     // Enable the specified EventPipe session.
     static bool EnableInternal(
         EventPipeSession *const pSession,
-        EventPipeProviderCallbackDataQueue *pEventPipeProviderCallbackDataQueue);
+        EventPipeProviderCallbackDataQueue *pEventPipeProviderCallbackDataQueue,
+        const bool enableSampleProfiler = true);
 
     // Callback function for the stack walker.  For each frame walked, this callback is invoked.
     static StackWalkAction StackWalkCallback(CrawlFrame *pCf, StackContents *pData);
@@ -226,6 +229,7 @@ private:
     static unsigned int * s_pProcGroupOffsets;
 #endif
     static Volatile<uint32_t> s_numberOfSessions;
+    static bool s_enableSampleProfilerAtStartup;
 };
 
 static_assert(EventPipe::MaxNumberOfSessions == 64, "Maximum number of EventPipe sessions is not 64.");
diff --git a/src/coreclr/src/vm/sampleprofiler.cpp b/src/coreclr/src/vm/sampleprofiler.cpp
index 851b56f48c1..e3abbb4365e 100644
--- a/src/coreclr/src/vm/sampleprofiler.cpp
+++ b/src/coreclr/src/vm/sampleprofiler.cpp
@@ -60,7 +60,7 @@ void SampleProfiler::Initialize(EventPipeProviderCallbackDataQueue* pEventPipePr
     }
 }
 
-void SampleProfiler::Enable(EventPipeProviderCallbackDataQueue *pEventPipeProviderCallbackDataQueue)
+void SampleProfiler::Enable()
 {
     CONTRACTL
     {
diff --git a/src/coreclr/src/vm/sampleprofiler.h b/src/coreclr/src/vm/sampleprofiler.h
index d775fe08c68..5524193ed1b 100644
--- a/src/coreclr/src/vm/sampleprofiler.h
+++ b/src/coreclr/src/vm/sampleprofiler.h
@@ -27,7 +27,7 @@ public:
     static void Initialize(EventPipeProviderCallbackDataQueue* pEventPipeProviderCallbackDataQueue);
 
     // Enable profiling.
-    static void Enable(EventPipeProviderCallbackDataQueue *pEventPipeProviderCallbackDataQueue);
+    static void Enable();
 
     // Disable profiling.
     static void Disable();