EventPipe lock implementation (#82790)

* EventPipe lock implementation

* FB

diff --git a/src/coreclr/nativeaot/Runtime/Crst.h b/src/coreclr/nativeaot/Runtime/Crst.h
index b266716..297a496 100644 (file)
--- a/src/coreclr/nativeaot/Runtime/Crst.h
+++ b/src/coreclr/nativeaot/Runtime/Crst.h
@@ -26,6 +26,8 @@ enum CrstType
     CrstThreadStore,
     CrstCastCache,
     CrstYieldProcessorNormalized,
+    CrstEventPipe,
+    CrstEventPipeConfig,
 };
 
 enum CrstFlags

diff --git a/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp b/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp
index 14ae72c..fea284d 100644 (file)
--- a/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp
+++ b/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp
@@ -20,6 +20,8 @@
 #include "holder.h"
 #include "SpinLock.h"
 
+// Uses _rt_aot_lock_internal_t that has CrstStatic as a field
+// This is initialized at the beginning and EventPipe library requires the lock handle to be maintained by the runtime
 ep_rt_lock_handle_t _ep_rt_aot_config_lock_handle;
 CrstStatic _ep_rt_aot_config_lock;
 
@@ -147,7 +149,7 @@ ep_rt_aot_atomic_compare_exchange_size_t (volatile size_t *target, size_t expect
     return static_cast<size_t>(PalInterlockedCompareExchange64 ((volatile int64_t *)target, (int64_t)value, (int64_t)expected));
 #else
     return static_cast<size_t>(PalInterlockedCompareExchange ((volatile int32_t *)target, (int32_t)value, (int32_t)expected));
-#endif 
+#endif
 }
 
 ep_char8_t *
@@ -351,7 +353,12 @@ ep_rt_aot_spin_lock_alloc (ep_rt_spin_lock_handle_t *spin_lock)
 {
     STATIC_CONTRACT_NOTHROW;
 
-    spin_lock->lock = new (nothrow) SpinLock ();
+    // EventPipe library expects SpinLocks to be used but NativeAOT will use a lock and change as needed if performance is an issue
+    // Uses _rt_aot_lock_internal_t that has CrstStatic as a field
+    // EventPipe library will intialize using thread, EventPipeBufferManager instances and will maintain these on the EventPipe library side
+
+    spin_lock->lock = new (nothrow) CrstStatic ();
+    spin_lock->lock->InitNoThrow (CrstType::CrstEventPipe);
 }
 
 void
@@ -502,4 +509,77 @@ ep_rt_aot_volatile_store_ptr_without_barrier (
     VolatileStoreWithoutBarrier<void *> ((void **)ptr, value);
 }
 
+void ep_rt_aot_init (void)
+{
+    extern ep_rt_lock_handle_t _ep_rt_aot_config_lock_handle;
+    extern CrstStatic _ep_rt_aot_config_lock;
+
+    _ep_rt_aot_config_lock_handle.lock = &_ep_rt_aot_config_lock;
+    _ep_rt_aot_config_lock_handle.lock->InitNoThrow (CrstType::CrstEventPipeConfig);
+}
+
+bool ep_rt_aot_lock_acquire (ep_rt_lock_handle_t *lock)
+{
+    if (lock) {
+        lock->lock->Enter();
+        return true;
+    }
+    return false;
+}
+
+bool ep_rt_aot_lock_release (ep_rt_lock_handle_t *lock)
+{
+    if (lock) {
+        lock->lock->Leave();
+        return true;
+    }
+    return false;
+}
+
+bool ep_rt_aot_spin_lock_acquire (ep_rt_spin_lock_handle_t *spin_lock)
+{
+    // In NativeAOT, we use a lock, instead of a SpinLock. 
+    // The method signature matches the EventPipe library expectation of a SpinLock
+    if (spin_lock) {
+        spin_lock->lock->Enter();
+        return true;
+    }
+    return false;
+}
+
+bool ep_rt_aot_spin_lock_release (ep_rt_spin_lock_handle_t *spin_lock)
+{
+    // In NativeAOT, we use a lock, instead of a SpinLock. 
+    // The method signature matches the EventPipe library expectation of a SpinLock
+    if (spin_lock) {
+        spin_lock->lock->Leave();
+        return true;
+    }
+    return false;
+}
+
+#ifdef EP_CHECKED_BUILD
+
+void ep_rt_aot_lock_requires_lock_held (const ep_rt_lock_handle_t *lock)
+{
+    EP_ASSERT (((ep_rt_lock_handle_t *)lock)->lock->OwnedByCurrentThread ());
+}
+
+void ep_rt_aot_lock_requires_lock_not_held (const ep_rt_lock_handle_t *lock)
+{
+    EP_ASSERT (lock->lock == NULL || !((ep_rt_lock_handle_t *)lock)->lock->OwnedByCurrentThread ());
+}
+
+void ep_rt_aot_spin_lock_requires_lock_held (const ep_rt_spin_lock_handle_t *spin_lock)
+{
+    EP_ASSERT (ep_rt_spin_lock_is_valid (spin_lock));
+       EP_ASSERT (spin_lock->lock->OwnedByCurrentThread ());
+}
+
+void ep_rt_aot_spin_lock_requires_lock_not_held (const ep_rt_spin_lock_handle_t *spin_lock)
+{
+       EP_ASSERT (spin_lock->lock == NULL || !spin_lock->lock->OwnedByCurrentThread ());
+}
+
+#endif /* EP_CHECKED_BUILD */
 #endif /* ENABLE_PERFTRACING */

diff --git a/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h b/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h
index 35494b2..50acdb9 100644 (file)
--- a/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h
+++ b/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h
@@ -1133,9 +1133,8 @@ inline
 ep_rt_lock_handle_t *
 ep_rt_aot_config_lock_get (void)
 {
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Implement EventPipe locking for NativeAOT
-    return nullptr;
+    extern ep_rt_lock_handle_t _ep_rt_aot_config_lock_handle;
+       return &_ep_rt_aot_config_lock_handle;
 }
 
 static
@@ -1320,8 +1319,8 @@ static
 void
 ep_rt_init (void) 
 {
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Implement EventPipe locking for NativeAOT
+    extern void ep_rt_aot_init (void);
+    ep_rt_aot_init();
 }
 
 static
@@ -1345,9 +1344,7 @@ inline
 bool
 ep_rt_config_acquire (void)
 {
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Implement EventPipe locking for NativeAOT
-    return true;   
+    return ep_rt_lock_acquire (ep_rt_aot_config_lock_get ());
 }
 
 static
@@ -1355,9 +1352,7 @@ inline
 bool
 ep_rt_config_release (void)
 {
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Implement EventPipe locking for NativeAOT
-    return true;
+    return ep_rt_lock_release (ep_rt_aot_config_lock_get ());
 }
 
 #ifdef EP_CHECKED_BUILD
@@ -1366,9 +1361,7 @@ inline
 void
 ep_rt_config_requires_lock_held (void)
 {
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Implement EventPipe locking for NativeAOT
-    return;
+    ep_rt_lock_requires_lock_held (ep_rt_aot_config_lock_get ());
 }
 
 static
@@ -1376,9 +1369,7 @@ inline
 void
 ep_rt_config_requires_lock_not_held (void)
 {
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Implement EventPipe locking for NativeAOT
-    return;
+    ep_rt_lock_requires_lock_not_held (ep_rt_aot_config_lock_get ());
 }
 #endif
 
@@ -2283,12 +2274,8 @@ bool
 ep_rt_lock_acquire (ep_rt_lock_handle_t *lock)
 {
     STATIC_CONTRACT_NOTHROW;
-
-    bool result = true;
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Implement EventPipe locking for NativeAOT 
-
-    return result;
+    extern bool ep_rt_aot_lock_acquire (ep_rt_lock_handle_t *lock);
+    return ep_rt_aot_lock_acquire(lock);
 }
 
 static
@@ -2296,12 +2283,8 @@ bool
 ep_rt_lock_release (ep_rt_lock_handle_t *lock)
 {
     STATIC_CONTRACT_NOTHROW;
-
-    bool result = true;
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Implement EventPipe locking for NativeAOT 
-
-    return result;
+    extern bool ep_rt_aot_lock_release (ep_rt_lock_handle_t *lock);
+    return ep_rt_aot_lock_release(lock);
 }
 
 #ifdef EP_CHECKED_BUILD
@@ -2312,7 +2295,8 @@ ep_rt_lock_requires_lock_held (const ep_rt_lock_handle_t *lock)
 {
 
     STATIC_CONTRACT_NOTHROW;
-    //EP_ASSERT (((ep_rt_lock_handle_t *)lock)->lock->OwnedByCurrentThread ());
+    extern void ep_rt_aot_lock_requires_lock_held (const ep_rt_lock_handle_t *lock);
+    ep_rt_aot_lock_requires_lock_held(lock);
 }
 
 static
@@ -2321,7 +2305,8 @@ void
 ep_rt_lock_requires_lock_not_held (const ep_rt_lock_handle_t *lock)
 {
     STATIC_CONTRACT_NOTHROW;
-    //EP_ASSERT (lock->lock == NULL || !((ep_rt_lock_handle_t *)lock)->lock->OwnedByCurrentThread ());
+    extern void ep_rt_aot_lock_requires_lock_not_held (const ep_rt_lock_handle_t *lock);
+    ep_rt_aot_lock_requires_lock_not_held(lock);
 }
 #endif
 
@@ -2334,8 +2319,7 @@ void
 ep_rt_spin_lock_alloc (ep_rt_spin_lock_handle_t *spin_lock)
 {
     STATIC_CONTRACT_NOTHROW;
-    extern void
-    ep_rt_aot_spin_lock_alloc (ep_rt_spin_lock_handle_t *spin_lock);
+    extern void ep_rt_aot_spin_lock_alloc (ep_rt_spin_lock_handle_t *spin_lock);
     ep_rt_aot_spin_lock_alloc(spin_lock);
 }
 
@@ -2345,8 +2329,7 @@ void
 ep_rt_spin_lock_free (ep_rt_spin_lock_handle_t *spin_lock)
 {
     STATIC_CONTRACT_NOTHROW;
-    extern void
-    ep_rt_aot_spin_lock_free (ep_rt_spin_lock_handle_t *spin_lock);
+    extern void ep_rt_aot_spin_lock_free (ep_rt_spin_lock_handle_t *spin_lock);
     ep_rt_aot_spin_lock_free(spin_lock);
 }
 
@@ -2356,12 +2339,9 @@ bool
 ep_rt_spin_lock_acquire (ep_rt_spin_lock_handle_t *spin_lock)
 {
     STATIC_CONTRACT_NOTHROW;
-//    EP_ASSERT (ep_rt_spin_lock_is_valid (spin_lock));
-
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Implement locking (maybe by making the manual Lock and Unlock functions public)
-    // SpinLock::Lock (*(spin_lock->lock));
-    return true;
+    EP_ASSERT (ep_rt_spin_lock_is_valid (spin_lock));
+    extern bool ep_rt_aot_spin_lock_acquire (ep_rt_spin_lock_handle_t *spin_lock);
+    return ep_rt_aot_spin_lock_acquire(spin_lock);
 }
 
 static
@@ -2371,11 +2351,8 @@ ep_rt_spin_lock_release (ep_rt_spin_lock_handle_t *spin_lock)
 {
     STATIC_CONTRACT_NOTHROW;
     EP_ASSERT (ep_rt_spin_lock_is_valid (spin_lock));
-
-    // shipping criteria: no EVENTPIPE-NATIVEAOT-TODO left in the codebase
-    // TODO: Implement locking (maybe by making the manual Lock and Unlock functions public)
-    // SpinLock::Unlock (*(spin_lock->lock));
-    return true;
+    extern bool ep_rt_aot_spin_lock_release (ep_rt_spin_lock_handle_t *spin_lock);
+    return ep_rt_aot_spin_lock_release(spin_lock);
 }
 
 #ifdef EP_CHECKED_BUILD
@@ -2385,8 +2362,8 @@ void
 ep_rt_spin_lock_requires_lock_held (const ep_rt_spin_lock_handle_t *spin_lock)
 {
     STATIC_CONTRACT_NOTHROW;
-    EP_ASSERT (ep_rt_spin_lock_is_valid (spin_lock));
-
+    extern void ep_rt_aot_spin_lock_requires_lock_held (const ep_rt_spin_lock_handle_t *spin_lock);
+    ep_rt_aot_spin_lock_requires_lock_held(spin_lock);
 }
 
 static
@@ -2395,7 +2372,8 @@ void
 ep_rt_spin_lock_requires_lock_not_held (const ep_rt_spin_lock_handle_t *spin_lock)
 {
     STATIC_CONTRACT_NOTHROW;
-
+    extern void ep_rt_aot_spin_lock_requires_lock_not_held (const ep_rt_spin_lock_handle_t *spin_lock);
+    ep_rt_aot_spin_lock_requires_lock_not_held(spin_lock);
 }
 #endif
 

diff --git a/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-types-aot.h b/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-types-aot.h
index 3219c86..647bb50 100644 (file)
--- a/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-types-aot.h
+++ b/src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-types-aot.h
@@ -138,11 +138,6 @@ struct _rt_aot_lock_internal_t {
     CrstStatic *lock;
 };
 
-class SpinLock;
-struct _rt_aot_spin_lock_internal_t {
-    SpinLock *lock;
-};
-
 /*
  * EventPipeBuffer.
  */
@@ -303,7 +298,8 @@ typedef struct _rt_aot_event_internal_t ep_rt_wait_event_handle_t;
 typedef struct _rt_aot_lock_internal_t ep_rt_lock_handle_t;
 
 #undef ep_rt_spin_lock_handle_t
-typedef _rt_aot_spin_lock_internal_t ep_rt_spin_lock_handle_t;
+// NativeAOT will start with CrstStatic instead of a SpinLock and change as needed if performance is an issue
+typedef struct _rt_aot_lock_internal_t ep_rt_spin_lock_handle_t;
 
 /*
  * Thread.

diff --git a/src/tests/tracing/eventpipe/simpleprovidervalidation/simpleprovidervalidation.cs b/src/tests/tracing/eventpipe/simpleprovidervalidation/simpleprovidervalidation.cs
index b6577fc..8d32997 100644 (file)
--- a/src/tests/tracing/eventpipe/simpleprovidervalidation/simpleprovidervalidation.cs
+++ b/src/tests/tracing/eventpipe/simpleprovidervalidation/simpleprovidervalidation.cs
@@ -44,14 +44,18 @@ namespace Tracing.Tests.SimpleProviderValidation
 
         private static Dictionary<string, ExpectedEventCount> _expectedEventCounts = new Dictionary<string, ExpectedEventCount>()
         {
-            { "MyEventSource", 1 },
+            { "MyEventSource", 100_000 },
             { "Microsoft-DotNETCore-EventPipe", 1}
         };
 
         private static Action _eventGeneratingAction = () => 
         {
-            Logger.logger.Log($"Firing an event...");
-            MyEventSource.Log.MyEvent();
+            for (int i = 0; i < 100_000; i++)
+            {
+                if (i % 10_000 == 0)
+                    Logger.logger.Log($"Fired MyEvent {i:N0}/100,000 times...");
+                MyEventSource.Log.MyEvent();
+            }
         };
     }
 }