From f2e87f65914eefd825464a18e59aeaae4dad69dc Mon Sep 17 00:00:00 2001 From: algorithmsarecool Date: Tue, 3 Sep 2024 21:16:13 -0500 Subject: [PATCH 1/8] Remove thread contention from Activity Start/Stop Author: algorithmsarecool --- .../src/System/Diagnostics/ActivitySource.cs | 139 +++++++----------- 1 file changed, 56 insertions(+), 83 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs index ebaddab4ec80cc..f371d0a6fa7323 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs @@ -407,105 +407,96 @@ internal void NotifyActivityAddException(Activity activity, Exception exception, } } - // SynchronizedList is a helper collection which ensure thread safety on the collection - // and allow enumerating the collection items and execute some action on the enumerated item and can detect any change in the collection - // during the enumeration which force restarting the enumeration again. - // Caution: We can have the action executed on the same item more than once which is ok in our scenarios. + //this class uses Interlocked operations and a copy-on-write design to ensure thread safety + //all operations are thread safe internal sealed class SynchronizedList { - private readonly List _list; - private uint _version; - - public SynchronizedList() => _list = new List(); + //This array must not be written to directly. Copy the array and then replace it with the new array. + private T[] _volatileArray; + public SynchronizedList() => _volatileArray = []; public void Add(T item) { - lock (_list) + while (true) { - _list.Add(item); - _version++; + T[] local = _volatileArray; + var newArray = new T[local.Length + 1]; + Array.Copy(local, newArray, local.Length); + newArray[local.Length] = item; + + if (Interlocked.CompareExchange(ref _volatileArray, newArray, local) == local) + break; } } public bool AddIfNotExist(T item) { - lock (_list) + while (true) { - if (!_list.Contains(item)) + T[] local = _volatileArray; + + foreach (T arrayItem in local) { - _list.Add(item); - _version++; - return true; + if (EqualityComparer.Default.Equals(arrayItem, item)) + { + return false; + } } - return false; + + // We didn't find the item in the list, so we can add it. + Add(item); + return true; } } public bool Remove(T item) { - lock (_list) + while (true) { - if (_list.Remove(item)) + T[] local = _volatileArray; + + for (int i = 0; i < local.Length; i++) { - _version++; - return true; + if (EqualityComparer.Default.Equals(local[i], item)) + { + var newArray = new T[local.Length - 1]; + Array.Copy(local, newArray, i); + Array.Copy(local, i + 1, newArray, i, local.Length - i - 1); + + if (Interlocked.CompareExchange(ref _volatileArray, newArray, local) == local) + return true; + break; + } } + return false; } } - public int Count => _list.Count; - - public void EnumWithFunc(ActivitySource.Function func, ref ActivityCreationOptions data, ref ActivitySamplingResult samplingResult, ref ActivityCreationOptions dataWithContext) + public int Count { - uint version = _version; - int index = 0; - - while (index < _list.Count) + get { - T item; - lock (_list) - { - if (version != _version) - { - version = _version; - index = 0; - continue; - } + T[] localArray = Volatile.Read(in _volatileArray); - item = _list[index]; - index++; - } + return localArray.Length; + } + } - // Important to call the func outside the lock. - // This is the whole point we are having this wrapper class. + public void EnumWithFunc(ActivitySource.Function func, ref ActivityCreationOptions data, ref ActivitySamplingResult samplingResult, ref ActivityCreationOptions dataWithContext) + { + T[] localArray = Volatile.Read(in _volatileArray); + foreach (T item in localArray) + { func(item, ref data, ref samplingResult, ref dataWithContext); } } public void EnumWithAction(Action action, object arg) { - uint version = _version; - int index = 0; - - while (index < _list.Count) + T[] localArray = Volatile.Read(in _volatileArray); + foreach (T item in localArray) { - T item; - lock (_list) - { - if (version != _version) - { - version = _version; - index = 0; - continue; - } - - item = _list[index]; - index++; - } - - // Important to call the action outside the lock. - // This is the whole point we are having this wrapper class. action(item, arg); } } @@ -517,29 +508,11 @@ public void EnumWithExceptionNotification(Activity activity, Exception exception return; } - uint version = _version; - int index = 0; - - while (index < _list.Count) + T[] localArray = Volatile.Read(in _volatileArray); + foreach (T item in localArray) { - T item; - lock (_list) - { - if (version != _version) - { - version = _version; - index = 0; - continue; - } - - item = _list[index]; - index++; - } - - // Important to notify outside the lock. - // This is the whole point we are having this wrapper class. (item as ActivityListener)!.ExceptionRecorder?.Invoke(activity, exception, ref tags); } } } -} +} \ No newline at end of file From ac471009d02d8bf4cbb6aa530bbf64cf81558af9 Mon Sep 17 00:00:00 2001 From: algorithmsarecool Date: Tue, 3 Sep 2024 21:40:20 -0500 Subject: [PATCH 2/8] Fix ref parameters and whitespace Author: algorithmsarecool --- .../src/System/Diagnostics/ActivitySource.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs index f371d0a6fa7323..7a5c5f2a2d3560 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs @@ -477,7 +477,7 @@ public int Count { get { - T[] localArray = Volatile.Read(in _volatileArray); + T[] localArray = Volatile.Read(ref _volatileArray); return localArray.Length; } @@ -485,7 +485,7 @@ public int Count public void EnumWithFunc(ActivitySource.Function func, ref ActivityCreationOptions data, ref ActivitySamplingResult samplingResult, ref ActivityCreationOptions dataWithContext) { - T[] localArray = Volatile.Read(in _volatileArray); + T[] localArray = Volatile.Read(ref _volatileArray); foreach (T item in localArray) { func(item, ref data, ref samplingResult, ref dataWithContext); @@ -494,7 +494,7 @@ public void EnumWithFunc(ActivitySource.Function func, ref public void EnumWithAction(Action action, object arg) { - T[] localArray = Volatile.Read(in _volatileArray); + T[] localArray = Volatile.Read(ref _volatileArray); foreach (T item in localArray) { action(item, arg); @@ -508,11 +508,11 @@ public void EnumWithExceptionNotification(Activity activity, Exception exception return; } - T[] localArray = Volatile.Read(in _volatileArray); + T[] localArray = Volatile.Read(ref _volatileArray); foreach (T item in localArray) { (item as ActivityListener)!.ExceptionRecorder?.Invoke(activity, exception, ref tags); } } } -} \ No newline at end of file +} From e7cf794497842e46a73ff2bea03ad3cbb25119ef Mon Sep 17 00:00:00 2001 From: algorithmsarecool Date: Wed, 4 Sep 2024 10:04:25 -0500 Subject: [PATCH 3/8] PR feedback. - Reduce duplication - add comments and make code more obvious - Use IndexOf Author: algorithmsarecool --- .../src/System/Diagnostics/ActivitySource.cs | 81 +++++++++++++------ 1 file changed, 56 insertions(+), 25 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs index 7a5c5f2a2d3560..1e9a9e3f6e10de 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs @@ -420,12 +420,15 @@ public void Add(T item) while (true) { T[] local = _volatileArray; - var newArray = new T[local.Length + 1]; - Array.Copy(local, newArray, local.Length); - newArray[local.Length] = item; - if (Interlocked.CompareExchange(ref _volatileArray, newArray, local) == local) - break; + if (TryAppendItemAndSwap(item, local)) + { + return; + } + else + { + //implicit continue + } } } @@ -435,17 +438,21 @@ public bool AddIfNotExist(T item) { T[] local = _volatileArray; - foreach (T arrayItem in local) + int index = Array.IndexOf(local, item); + + if (index >= 0) { - if (EqualityComparer.Default.Equals(arrayItem, item)) - { - return false; - } + return false; } - // We didn't find the item in the list, so we can add it. - Add(item); - return true; + if (TryAppendItemAndSwap(item, local)) + { + return true; + } + else + { + //implicit continue + } } } @@ -455,21 +462,21 @@ public bool Remove(T item) { T[] local = _volatileArray; - for (int i = 0; i < local.Length; i++) - { - if (EqualityComparer.Default.Equals(local[i], item)) - { - var newArray = new T[local.Length - 1]; - Array.Copy(local, newArray, i); - Array.Copy(local, i + 1, newArray, i, local.Length - i - 1); + int index = Array.IndexOf(local, item); - if (Interlocked.CompareExchange(ref _volatileArray, newArray, local) == local) - return true; - break; - } + if (index < 0) + { + return false; } - return false; + if (TryRemoveIndexAndSwap(index, local)) + { + return true; + } + else + { + //implicit continue + } } } @@ -514,5 +521,29 @@ public void EnumWithExceptionNotification(Activity activity, Exception exception (item as ActivityListener)!.ExceptionRecorder?.Invoke(activity, exception, ref tags); } } + + private bool TryAppendItemAndSwap(T item, T[] localCopy) + { + T[] newArray = new T[localCopy.Length + 1]; + + Array.Copy(localCopy, newArray, localCopy.Length);//copy existing items + newArray[localCopy.Length] = item;//copy new item + + return Interlocked.CompareExchange(ref _volatileArray, newArray, localCopy) == localCopy; + } + + private bool TryRemoveIndexAndSwap(int index, T[] localCopy) + { + T[] newArray = new T[localCopy.Length - 1]; + + Array.Copy(localCopy, newArray, index);//copy existing items before index + + Array.Copy( + localCopy, index + 1, //position after the index, skipping it + newArray, index, localCopy.Length - index - 1//remaining items accounting for removed item + ); + + return Interlocked.CompareExchange(ref _volatileArray, newArray, localCopy) == localCopy; + } } } From 57851543ea969dd137d43004f2e372a85c7eb40b Mon Sep 17 00:00:00 2001 From: algorithmsarecool Date: Thu, 5 Sep 2024 19:25:20 -0500 Subject: [PATCH 4/8] PR feedback to simplify locking strategy --- .../src/System/Diagnostics/ActivitySource.cs | 118 +++++++----------- 1 file changed, 42 insertions(+), 76 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs index 1e9a9e3f6e10de..ad87a8bb86c536 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs @@ -407,93 +407,85 @@ internal void NotifyActivityAddException(Activity activity, Exception exception, } } - //this class uses Interlocked operations and a copy-on-write design to ensure thread safety - //all operations are thread safe + //this class uses a copy-on-write design to ensure thread safety all operations are thread safe. + //However, it is possible for read-only operations to see stale versions of the item while a change + //is occurring. This should not be a practical issue if updates are infrequent internal sealed class SynchronizedList { - //This array must not be written to directly. Copy the array and then replace it with the new array. + private readonly object _writeLock; + //This array must not be mutated directly. To mutate, obtain the lock, copy the array and then replace it with the new array. private T[] _volatileArray; - public SynchronizedList() => _volatileArray = []; + public SynchronizedList() + { + _volatileArray = []; + _writeLock = new(); + } public void Add(T item) { - while (true) + lock (_writeLock) { - T[] local = _volatileArray; + T[] newArray = new T[_volatileArray.Length + 1]; - if (TryAppendItemAndSwap(item, local)) - { - return; - } - else - { - //implicit continue - } + Array.Copy(_volatileArray, newArray, _volatileArray.Length);//copy existing items + newArray[_volatileArray.Length] = item;//copy new item + + _volatileArray = newArray; } } public bool AddIfNotExist(T item) { - while (true) + lock (_writeLock) { - T[] local = _volatileArray; - - int index = Array.IndexOf(local, item); + int index = Array.IndexOf(_volatileArray, item); if (index >= 0) { return false; } - if (TryAppendItemAndSwap(item, local)) - { - return true; - } - else - { - //implicit continue - } + T[] newArray = new T[_volatileArray.Length + 1]; + + Array.Copy(_volatileArray, newArray, _volatileArray.Length);//copy existing items + newArray[_volatileArray.Length] = item;//copy new item + + _volatileArray = newArray; + + return true; } } public bool Remove(T item) { - while (true) + lock (_writeLock) { - T[] local = _volatileArray; - - int index = Array.IndexOf(local, item); + int index = Array.IndexOf(_volatileArray, item); if (index < 0) { return false; } - if (TryRemoveIndexAndSwap(index, local)) - { - return true; - } - else - { - //implicit continue - } - } - } + T[] newArray = new T[_volatileArray.Length - 1]; - public int Count - { - get - { - T[] localArray = Volatile.Read(ref _volatileArray); + Array.Copy(_volatileArray, newArray, index);//copy existing items before index - return localArray.Length; + Array.Copy( + _volatileArray, index + 1, //position after the index, skipping it + newArray, index, _volatileArray.Length - index - 1//remaining items accounting for removed item + ); + + _volatileArray = newArray; + return true; } } + public int Count => _volatileArray.Length; + public void EnumWithFunc(ActivitySource.Function func, ref ActivityCreationOptions data, ref ActivitySamplingResult samplingResult, ref ActivityCreationOptions dataWithContext) { - T[] localArray = Volatile.Read(ref _volatileArray); - foreach (T item in localArray) + foreach (T item in _volatileArray) { func(item, ref data, ref samplingResult, ref dataWithContext); } @@ -501,8 +493,7 @@ public void EnumWithFunc(ActivitySource.Function func, ref public void EnumWithAction(Action action, object arg) { - T[] localArray = Volatile.Read(ref _volatileArray); - foreach (T item in localArray) + foreach (T item in _volatileArray) { action(item, arg); } @@ -515,35 +506,10 @@ public void EnumWithExceptionNotification(Activity activity, Exception exception return; } - T[] localArray = Volatile.Read(ref _volatileArray); - foreach (T item in localArray) + foreach (T item in _volatileArray) { (item as ActivityListener)!.ExceptionRecorder?.Invoke(activity, exception, ref tags); } } - - private bool TryAppendItemAndSwap(T item, T[] localCopy) - { - T[] newArray = new T[localCopy.Length + 1]; - - Array.Copy(localCopy, newArray, localCopy.Length);//copy existing items - newArray[localCopy.Length] = item;//copy new item - - return Interlocked.CompareExchange(ref _volatileArray, newArray, localCopy) == localCopy; - } - - private bool TryRemoveIndexAndSwap(int index, T[] localCopy) - { - T[] newArray = new T[localCopy.Length - 1]; - - Array.Copy(localCopy, newArray, index);//copy existing items before index - - Array.Copy( - localCopy, index + 1, //position after the index, skipping it - newArray, index, localCopy.Length - index - 1//remaining items accounting for removed item - ); - - return Interlocked.CompareExchange(ref _volatileArray, newArray, localCopy) == localCopy; - } } } From fdeac49d279e7131ac1ad746cdfdc8474d6a549b Mon Sep 17 00:00:00 2001 From: algorithmsarecool Date: Thu, 5 Sep 2024 20:04:48 -0500 Subject: [PATCH 5/8] PR feedback, final nits --- .../src/System/Diagnostics/ActivitySource.cs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs index ad87a8bb86c536..f82e54e65d5b5e 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs @@ -407,13 +407,13 @@ internal void NotifyActivityAddException(Activity activity, Exception exception, } } - //this class uses a copy-on-write design to ensure thread safety all operations are thread safe. - //However, it is possible for read-only operations to see stale versions of the item while a change - //is occurring. This should not be a practical issue if updates are infrequent + // This class uses a copy-on-write design to ensure thread safety all operations are thread safe. + // However, it is possible for read-only operations to see stale versions of the item while a change + // is occurring. internal sealed class SynchronizedList { private readonly object _writeLock; - //This array must not be mutated directly. To mutate, obtain the lock, copy the array and then replace it with the new array. + // This array must not be mutated directly. To mutate, obtain the lock, copy the array and then replace it with the new array. private T[] _volatileArray; public SynchronizedList() { @@ -427,8 +427,8 @@ public void Add(T item) { T[] newArray = new T[_volatileArray.Length + 1]; - Array.Copy(_volatileArray, newArray, _volatileArray.Length);//copy existing items - newArray[_volatileArray.Length] = item;//copy new item + Array.Copy(_volatileArray, newArray, _volatileArray.Length);// copy existing items + newArray[_volatileArray.Length] = item;// copy new item _volatileArray = newArray; } @@ -447,8 +447,8 @@ public bool AddIfNotExist(T item) T[] newArray = new T[_volatileArray.Length + 1]; - Array.Copy(_volatileArray, newArray, _volatileArray.Length);//copy existing items - newArray[_volatileArray.Length] = item;//copy new item + Array.Copy(_volatileArray, newArray, _volatileArray.Length);// copy existing items + newArray[_volatileArray.Length] = item;// copy new item _volatileArray = newArray; @@ -469,11 +469,11 @@ public bool Remove(T item) T[] newArray = new T[_volatileArray.Length - 1]; - Array.Copy(_volatileArray, newArray, index);//copy existing items before index + Array.Copy(_volatileArray, newArray, index);// copy existing items before index Array.Copy( - _volatileArray, index + 1, //position after the index, skipping it - newArray, index, _volatileArray.Length - index - 1//remaining items accounting for removed item + _volatileArray, index + 1, // position after the index, skipping it + newArray, index, _volatileArray.Length - index - 1// remaining items accounting for removed item ); _volatileArray = newArray; From 847ce4464631c326d8407344eda90bf35676f74a Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 29 Oct 2024 15:06:03 -0700 Subject: [PATCH 6/8] package authoring --- .../src/System.Diagnostics.DiagnosticSource.csproj | 2 ++ 1 file changed, 2 insertions(+) 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 2316c532efb58b..bec1621b5f2b71 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj @@ -7,6 +7,8 @@ false false true + true + true Provides Classes that allow you to decouple code logging rich (unserializable) diagnostics/telemetry (e.g. framework) from code that consumes it (e.g. tools) Commonly Used Types: From 9b3d1ef65664fc26b99189feb5a5f2f8d8ccc812 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 29 Oct 2024 15:08:21 -0700 Subject: [PATCH 7/8] Fix package authoring --- .../src/System.Diagnostics.DiagnosticSource.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 bec1621b5f2b71..935b7322b46b25 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj @@ -7,7 +7,7 @@ false false true - true + 1 true Provides Classes that allow you to decouple code logging rich (unserializable) diagnostics/telemetry (e.g. framework) from code that consumes it (e.g. tools) From fddc585aec657506c1284d48ad4a4b98b8bae302 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 30 Oct 2024 11:26:32 -0700 Subject: [PATCH 8/8] revert package authoring as it is not needed anymore in net9.0 --- .../src/System.Diagnostics.DiagnosticSource.csproj | 2 -- 1 file changed, 2 deletions(-) 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 935b7322b46b25..2316c532efb58b 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj @@ -7,8 +7,6 @@ false false true - 1 - true Provides Classes that allow you to decouple code logging rich (unserializable) diagnostics/telemetry (e.g. framework) from code that consumes it (e.g. tools) Commonly Used Types: