From: Lakshan Fernando Date: Thu, 2 Mar 2023 17:08:32 +0000 (-0800) Subject: EventPipe lock implementation (#82790) X-Git-Tag: accepted/tizen/unified/riscv/20231226.055536~3715 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=cc8f02a08a8e786992c15645b19bfdbcb0a36187;p=platform%2Fupstream%2Fdotnet%2Fruntime.git 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 --- 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 --- 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(PalInterlockedCompareExchange64 ((volatile int64_t *)target, (int64_t)value, (int64_t)expected)); #else return static_cast(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 **)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 --- 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 --- 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 --- 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 _expectedEventCounts = new Dictionary() { - { "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(); + } }; } }