From 981a85989d49daee6b2147113b7de639f5e5d903 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 5 Nov 2024 13:07:28 -0800 Subject: [PATCH] [release/9.0-staging] Remove thread contention from Activity Start/Stop (#109359) * Remove thread contention from Activity Start/Stop Author: algorithmsarecool * Fix ref parameters and whitespace Author: algorithmsarecool * PR feedback. - Reduce duplication - add comments and make code more obvious - Use IndexOf Author: algorithmsarecool * PR feedback to simplify locking strategy * PR feedback, final nits * package authoring * Fix package authoring * revert package authoring as it is not needed anymore in net9.0 --------- Co-authored-by: algorithmsarecool Co-authored-by: Tarek Mahmoud Sayed --- .../src/System/Diagnostics/ActivitySource.cs | 136 +++++++----------- 1 file changed, 53 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..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,105 +407,94 @@ 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 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 List _list; - private uint _version; - - public SynchronizedList() => _list = new List(); + 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 = []; + _writeLock = new(); + } public void Add(T item) { - lock (_list) + lock (_writeLock) { - _list.Add(item); - _version++; + 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; } } public bool AddIfNotExist(T item) { - lock (_list) + lock (_writeLock) { - if (!_list.Contains(item)) + int index = Array.IndexOf(_volatileArray, item); + + if (index >= 0) { - _list.Add(item); - _version++; - return true; + return false; } - return false; + + 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) { - lock (_list) + lock (_writeLock) { - if (_list.Remove(item)) + int index = Array.IndexOf(_volatileArray, item); + + if (index < 0) { - _version++; - return true; + return false; } - return false; + + T[] newArray = new T[_volatileArray.Length - 1]; + + 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 = newArray; + return true; } } - public int Count => _list.Count; + public int Count => _volatileArray.Length; public void EnumWithFunc(ActivitySource.Function func, ref ActivityCreationOptions data, ref ActivitySamplingResult samplingResult, ref ActivityCreationOptions dataWithContext) { - uint version = _version; - int index = 0; - - while (index < _list.Count) + foreach (T item in _volatileArray) { - T item; - lock (_list) - { - if (version != _version) - { - version = _version; - index = 0; - continue; - } - - item = _list[index]; - index++; - } - - // Important to call the func outside the lock. - // This is the whole point we are having this wrapper class. 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) + foreach (T item in _volatileArray) { - 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,27 +506,8 @@ public void EnumWithExceptionNotification(Activity activity, Exception exception return; } - uint version = _version; - int index = 0; - - while (index < _list.Count) + foreach (T item in _volatileArray) { - 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); } }