From 6fb2af6540ad5af408fdbe60c077c2bd4dfdcce0 Mon Sep 17 00:00:00 2001 From: hendrik <73221731+hendriklhf@users.noreply.github.com> Date: Fri, 11 Apr 2025 15:57:45 +0200 Subject: [PATCH 1/3] Added missing clearing of pooled arrays --- .../System/Diagnostics/Metrics/Measurement.cs | 5 +++++ .../Collections/Generic/ValueListBuilder.cs | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Measurement.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Measurement.cs index f446960d647739..d19d86a5a88025 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Measurement.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Measurement.cs @@ -145,6 +145,8 @@ public Measurement(T value, in TagList tags) result = new KeyValuePair[count]; array.AsSpan().Slice(0, count).CopyTo(result.AsSpan()); + Array.Clear(array, 0, count); + // Note that we don't include the return of the array inside a finally block per the established guidelines for ArrayPool. // We don't expect the above to throw an exception, but if it does it would be infrequent and the GC will collect the array. ArrayPool>.Shared.Return(array); @@ -155,7 +157,10 @@ static void Grow(ref KeyValuePair[] array, ref int length) { KeyValuePair[] newArray = ArrayPool>.Shared.Rent(length * 2); array.CopyTo(newArray, 0); + + Array.Clear(array, 0, array.Length); ArrayPool>.Shared.Return(array); + array = newArray; length = array.Length; } diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs index 8c84415aaea093..a9deceab6bee60 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs @@ -165,6 +165,16 @@ public void Dispose() if (toReturn != null) { _arrayFromPool = null; + +#if NET || NETSTANDARD2_1_OR_GREATER + if (RuntimeHelpers.IsReferenceOrContainsReferences()) +#else + if (!typeof(T).IsPrimitive) +#endif + { + Array.Clear(toReturn, 0, _pos); + } + ArrayPool.Shared.Return(toReturn); } } @@ -200,6 +210,15 @@ private void Grow(int additionalCapacityRequired = 1) _span = _arrayFromPool = array; if (toReturn != null) { +#if NET || NETSTANDARD2_1_OR_GREATER + if (RuntimeHelpers.IsReferenceOrContainsReferences()) +#else + if (!typeof(T).IsPrimitive) +#endif + { + Array.Clear(toReturn, 0, _pos); + } + ArrayPool.Shared.Return(toReturn); } } From 05c8b8ea671815bb4d70ada91dba50f60dfbe852 Mon Sep 17 00:00:00 2001 From: hendriklhf <73221731+hendriklhf@users.noreply.github.com> Date: Tue, 22 Apr 2025 19:06:01 +0200 Subject: [PATCH 2/3] using Linq's ToArray method --- ...System.Diagnostics.DiagnosticSource.csproj | 1 + .../System/Diagnostics/Metrics/Measurement.cs | 71 +------------------ .../tests/MetricsTests.cs | 4 ++ 3 files changed, 7 insertions(+), 69 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 bbd90f7f5ee0ec..9fb48dbfd83a6f 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj @@ -151,6 +151,7 @@ System.Diagnostics.DiagnosticSource + diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Measurement.cs b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Measurement.cs index d19d86a5a88025..42fbaeca7cbc71 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Measurement.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Measurement.cs @@ -3,7 +3,7 @@ using System.Buffers; using System.Collections.Generic; -using System.Runtime.CompilerServices; +using System.Linq; namespace System.Diagnostics.Metrics { @@ -32,7 +32,7 @@ public Measurement(T value) /// The tags associated with the measurement. public Measurement(T value, IEnumerable>? tags) { - _tags = ToArray(tags); + _tags = tags?.ToArray() ?? Instrument.EmptyTags; Value = value; } @@ -98,72 +98,5 @@ public Measurement(T value, in TagList tags) /// Gets the value of the measurement. /// public T Value { get; } - - // Private helper to copy IEnumerable to array. We have it to avoid adding dependencies on System.Linq - private static KeyValuePair[] ToArray(IEnumerable>? tags) - { - if (tags is null) - return Instrument.EmptyTags; - - KeyValuePair[] result; - - // When the input is a collection, we can allocate a correctly sized array and copy directly into it. - if (tags is ICollection> collection) - { - int items = collection.Count; - - if (items == 0) - return Instrument.EmptyTags; - - result = new KeyValuePair[items]; - collection.CopyTo(result, 0); - return result; - } - - // In any other case, we must enumerate the input. - // We use a pooled array as a buffer to avoid allocating until we know the final size we need. - // This assumes that there are 32 or fewer tags, which is a reasonable assumption for most cases. - // In the worst case, we will grow the buffer by renting a larger array. - KeyValuePair[] array = ArrayPool>.Shared.Rent(32); - int count = 0; - int length = array.Length; - - foreach (KeyValuePair item in tags) - { - if (count == length) - Grow(ref array, ref length); - - array[count++] = item; - } - - if (count == 0) - { - ArrayPool>.Shared.Return(array); - return Instrument.EmptyTags; - } - - result = new KeyValuePair[count]; - array.AsSpan().Slice(0, count).CopyTo(result.AsSpan()); - - Array.Clear(array, 0, count); - - // Note that we don't include the return of the array inside a finally block per the established guidelines for ArrayPool. - // We don't expect the above to throw an exception, but if it does it would be infrequent and the GC will collect the array. - ArrayPool>.Shared.Return(array); - return result; - - [MethodImpl(MethodImplOptions.AggressiveInlining)] - static void Grow(ref KeyValuePair[] array, ref int length) - { - KeyValuePair[] newArray = ArrayPool>.Shared.Rent(length * 2); - array.CopyTo(newArray, 0); - - Array.Clear(array, 0, array.Length); - ArrayPool>.Shared.Return(array); - - array = newArray; - length = array.Length; - } - } } } diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs b/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs index 05bd3aed1bd0e1..3a83e2ba346257 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs +++ b/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs @@ -26,6 +26,10 @@ public void MeasurementConstructionTest() Assert.Equal(i, measurement.Value); TagListTests.ValidateTags(new TagList(measurement.Tags), tagsArray); + measurement = new Measurement(i, tagsArray.AsEnumerable()); + Assert.Equal(i, measurement.Value); + TagListTests.ValidateTags(new TagList(measurement.Tags), tagsArray); + measurement = new Measurement(i, tagsArray); Assert.Equal(i, measurement.Value); TagListTests.ValidateTags(new TagList(measurement.Tags), tagsArray); From 8fa44fcae64c5b4f35cf4cfd066b0bc1e61bf571 Mon Sep 17 00:00:00 2001 From: hendriklhf <73221731+hendriklhf@users.noreply.github.com> Date: Tue, 22 Apr 2025 19:07:22 +0200 Subject: [PATCH 3/3] added Return(T[], int) overload for ArrayPool --- .../src/System/Buffers/ArrayPool.cs | 6 +++++ .../Collections/Generic/ValueListBuilder.cs | 22 +++++++++++++++---- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Buffers/ArrayPool.cs b/src/libraries/System.Private.CoreLib/src/System/Buffers/ArrayPool.cs index 7ea932f3bd8037..9f6fcc8ac076bc 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Buffers/ArrayPool.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Buffers/ArrayPool.cs @@ -97,5 +97,11 @@ public static ArrayPool Create(int maxArrayLength, int maxArraysPerBucket) => /// if it's determined that the pool already has enough buffers stored. /// public abstract void Return(T[] array, bool clearArray = false); + + internal void Return(T[] array, int lengthToClear) + { + array.AsSpan(0, lengthToClear).Clear(); + Return(array); + } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs index a9deceab6bee60..dbdf8b157f2b68 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs @@ -166,16 +166,23 @@ public void Dispose() { _arrayFromPool = null; -#if NET || NETSTANDARD2_1_OR_GREATER +#if SYSTEM_PRIVATE_CORELIB if (RuntimeHelpers.IsReferenceOrContainsReferences()) + { + ArrayPool.Shared.Return(toReturn, _pos); + } + else + { + ArrayPool.Shared.Return(toReturn); + } #else if (!typeof(T).IsPrimitive) -#endif { Array.Clear(toReturn, 0, _pos); } ArrayPool.Shared.Return(toReturn); +#endif } } @@ -210,16 +217,23 @@ private void Grow(int additionalCapacityRequired = 1) _span = _arrayFromPool = array; if (toReturn != null) { -#if NET || NETSTANDARD2_1_OR_GREATER +#if SYSTEM_PRIVATE_CORELIB if (RuntimeHelpers.IsReferenceOrContainsReferences()) + { + ArrayPool.Shared.Return(toReturn, _pos); + } + else + { + ArrayPool.Shared.Return(toReturn); + } #else if (!typeof(T).IsPrimitive) -#endif { Array.Clear(toReturn, 0, _pos); } ArrayPool.Shared.Return(toReturn); +#endif } } }