From d5798799285c3e074863edcfef914d2f18e257b5 Mon Sep 17 00:00:00 2001 From: Janus Thorborg Date: Fri, 21 Jul 2023 12:53:50 +0200 Subject: [PATCH 01/15] Relaxing constraints of System.GC.AllocateArray. It's now possible to allocate pinned and default-initialized arrays that contain references. Fix #48703 --- src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs index d59ad132ef59ee..41357e87c80a61 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs @@ -775,18 +775,12 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = /// Specifies the type of the array element. /// Specifies the length of the array. /// Specifies whether the allocated array must be pinned. - /// - /// If pinned is set to true, must not be a reference type or a type that contains object references. - /// public static T[] AllocateArray(int length, bool pinned = false) // T[] rather than T?[] to match `new T[length]` behavior { GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_NO_FLAGS; if (pinned) { - if (RuntimeHelpers.IsReferenceOrContainsReferences()) - ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(T)); - flags = GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; } From 5fc91572348925742363f02d76ed791edc62a377 Mon Sep 17 00:00:00 2001 From: Janus Thorborg Date: Fri, 21 Jul 2023 12:59:59 +0200 Subject: [PATCH 02/15] Adding coverage of GC.AllocateArray with ref types. Added a test that pins a reference type array and resolves references to indices with pointer arithmetic, checking that modifications are visible back in the original array. Test #48703 --- .../System.Runtime/tests/System/GCTests.cs | 47 ++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Runtime/tests/System/GCTests.cs b/src/libraries/System.Runtime/tests/System/GCTests.cs index e59aea9ccdd1fb..d115fd4b6bd0be 100644 --- a/src/libraries/System.Runtime/tests/System/GCTests.cs +++ b/src/libraries/System.Runtime/tests/System/GCTests.cs @@ -1059,12 +1059,57 @@ private static void AllocateArrayTooLarge() } [Fact] - private static void AllocateArrayRefType() + private static void AllocateArrayUninitializedPinned_RefType_ThrowsArgumentException() { GC.AllocateUninitializedArray(100); Assert.Throws(() => GC.AllocateUninitializedArray(100, pinned: true)); } + struct EmbeddedValueType + { + public T Value; + } + + [Fact] + private static void AllocateArrayPinned_ManagedType_DoesNotThrow() + { + void TryType() + { + GC.AllocateArray(100); + GC.AllocateArray(100, pinned: true); + + GC.AllocateArray>(100); + GC.AllocateArray>(100, pinned: true); + } + + TryType(); + TryType(); + } + + [Fact] + private unsafe static void AllocateArrayPinned_ManagedValueType_CanRoundtripThroughPointer() + { + const int k_Length = 100; + var rng = new Random(0xAF); + + var array = GC.AllocateArray>(k_Length, pinned: true); + byte* pointer = (byte*)Unsafe.AsPointer(ref array[0]); + var size = Unsafe.SizeOf>(); + + for(int i = 0; i < k_Length; ++i) + { + var idx = rng.Next(k_Length); + ref var evt = ref Unsafe.AsRef>(pointer + size * idx); + + var stringValue = rng.NextSingle().ToString(); + evt.Value = stringValue; + + Assert.Equal(evt.Value, array[idx].Value); + } + + GC.KeepAlive(array); + } + [Fact] private unsafe static void AllocateArrayCheckPinning() { From 376fb8983f4ab92a6464212809e144932fef47b7 Mon Sep 17 00:00:00 2001 From: Janus Thorborg Date: Fri, 21 Jul 2023 13:47:23 +0200 Subject: [PATCH 03/15] Mono/NativeAOT relaxing of System.GC.AllocateArray. --- .../System.Private.CoreLib/src/System/GC.NativeAot.cs | 6 ------ src/mono/System.Private.CoreLib/src/System/GC.Mono.cs | 2 -- 2 files changed, 8 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs index a9bf5a945fb663..5555b4364924ba 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs @@ -851,18 +851,12 @@ static T[] AllocateNewUninitializedArray(int length, bool pinned) /// Specifies the type of the array element. /// Specifies the length of the array. /// Specifies whether the allocated array must be pinned. - /// - /// If pinned is set to true, must not be a reference type or a type that contains object references. - /// public static unsafe T[] AllocateArray(int length, bool pinned = false) { GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_NO_FLAGS; if (pinned) { - if (RuntimeHelpers.IsReferenceOrContainsReferences()) - ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(T)); - flags = GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; } diff --git a/src/mono/System.Private.CoreLib/src/System/GC.Mono.cs b/src/mono/System.Private.CoreLib/src/System/GC.Mono.cs index c87e0ee05c0f92..6d094d9bf9785d 100644 --- a/src/mono/System.Private.CoreLib/src/System/GC.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/GC.Mono.cs @@ -281,8 +281,6 @@ public static T[] AllocateUninitializedArray(int length, bool pinned = false) public static T[] AllocateArray(int length, bool pinned = false) { if (pinned) { - if (RuntimeHelpers.IsReferenceOrContainsReferences()) - ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(T)); return Unsafe.As(AllocPinnedArray(typeof(T[]), length)); } From ea6de358faf7284b38da47acf1439793cd2e4fce Mon Sep 17 00:00:00 2001 From: Janus Thorborg Date: Fri, 21 Jul 2023 17:18:27 +0200 Subject: [PATCH 04/15] Relaxed GC.AllocateUninitializedArray for ref types as well. This was done by deferring reference types to GC.AllocateArray to avoid potential memory issues with the GC + uninitialized memory. The API only promises to avoid initialization if possible, anyway. Refactored tests to parametrically exercise these new relaxed constraints. --- .../src/System/GC.CoreCLR.cs | 5 +--- .../src/System/GC.NativeAot.cs | 5 +--- .../System.Runtime/tests/System/GCTests.cs | 28 ++++++++----------- 3 files changed, 14 insertions(+), 24 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs index 41357e87c80a61..b3f8e2731aa79b 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs @@ -732,9 +732,6 @@ internal static void UnregisterMemoryLoadChangeNotification(Action notification) /// Specifies the type of the array element. /// Specifies the length of the array. /// Specifies whether the allocated array must be pinned. - /// - /// If pinned is set to true, must not be a reference type or a type that contains object references. - /// [MethodImpl(MethodImplOptions.AggressiveInlining)] // forced to ensure no perf drop for small memory buffers (hot path) public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = false) // T[] rather than T?[] to match `new T[length]` behavior { @@ -759,7 +756,7 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = } else if (RuntimeHelpers.IsReferenceOrContainsReferences()) { - ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(T)); + return AllocateArray(length, pinned: true); } GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL; diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs index 5555b4364924ba..d81a339cfa6fb6 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs @@ -795,9 +795,6 @@ internal static ulong GetSegmentSize() /// Specifies the type of the array element. /// Specifies the length of the array. /// Specifies whether the allocated array must be pinned. - /// - /// If pinned is set to true, must not be a reference type or a type that contains object references. - /// [MethodImpl(MethodImplOptions.AggressiveInlining)] // forced to ensure no perf drop for small memory buffers (hot path) public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = false) { @@ -821,7 +818,7 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = } else if (RuntimeHelpers.IsReferenceOrContainsReferences()) { - ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(T)); + return AllocateArray(length, pinned: true); } // kept outside of the small arrays hot path to have inlining without big size growth diff --git a/src/libraries/System.Runtime/tests/System/GCTests.cs b/src/libraries/System.Runtime/tests/System/GCTests.cs index d115fd4b6bd0be..66f38d4008ef96 100644 --- a/src/libraries/System.Runtime/tests/System/GCTests.cs +++ b/src/libraries/System.Runtime/tests/System/GCTests.cs @@ -1058,41 +1058,37 @@ private static void AllocateArrayTooLarge() Assert.Throws(() => GC.AllocateUninitializedArray(int.MaxValue, pinned: true)); } - [Fact] - private static void AllocateArrayUninitializedPinned_RefType_ThrowsArgumentException() - { - GC.AllocateUninitializedArray(100); - Assert.Throws(() => GC.AllocateUninitializedArray(100, pinned: true)); - } - struct EmbeddedValueType { + unsafe fixed byte _[7]; public T Value; } - [Fact] - private static void AllocateArrayPinned_ManagedType_DoesNotThrow() + [Theory] + [InlineData(false), InlineData(true)] + private static void AllocateArray_UninitializedOrNot_WithManagedType_DoesNotThrow(bool pinned) { void TryType() { - GC.AllocateArray(100); - GC.AllocateArray(100, pinned: true); + GC.AllocateUninitializedArray(100, pinned); + GC.AllocateArray(100, pinned); - GC.AllocateArray>(100); - GC.AllocateArray>(100, pinned: true); + GC.AllocateArray>(100, pinned); + GC.AllocateUninitializedArray>(100, pinned); } TryType(); TryType(); } - [Fact] - private unsafe static void AllocateArrayPinned_ManagedValueType_CanRoundtripThroughPointer() + [Theory] + [InlineData(false), InlineData(true)] + private unsafe static void AllocateArrayPinned_ManagedValueType_CanRoundtripThroughPointer(bool uninitialized) { const int k_Length = 100; var rng = new Random(0xAF); - var array = GC.AllocateArray>(k_Length, pinned: true); + var array = uninitialized ? GC.AllocateUninitializedArray>(k_Length, pinned: true) : GC.AllocateArray>(k_Length, pinned: true); byte* pointer = (byte*)Unsafe.AsPointer(ref array[0]); var size = Unsafe.SizeOf>(); From c4032663c4b359257310760c079ed4dfe2e6abb8 Mon Sep 17 00:00:00 2001 From: Janus Thorborg Date: Fri, 21 Jul 2023 19:34:41 +0200 Subject: [PATCH 05/15] Simplifiy conditionals in AllocateUninitializedArray. Relying on internal implementation zeroing refs if necessary. --- src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs index b3f8e2731aa79b..c80de78e4374ab 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs @@ -754,16 +754,12 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = #endif } - else if (RuntimeHelpers.IsReferenceOrContainsReferences()) - { - return AllocateArray(length, pinned: true); - } GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL; if (pinned) flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; - return Unsafe.As(AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(T[]).TypeHandle), length, flags)); + return Unsafe.As(AllocateNewArray(typeof(T[]).TypeHandle.Value, length, flags)); } /// From 30fd37522426dac18945c7518e70700289d13fa1 Mon Sep 17 00:00:00 2001 From: Janus Thorborg Date: Fri, 21 Jul 2023 19:38:43 +0200 Subject: [PATCH 06/15] Also simplify path for NativeOAT. Mono already piggybacks on the AllocateArray path anyway. --- .../System.Private.CoreLib/src/System/GC.NativeAot.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs index d81a339cfa6fb6..84cd270681e7c3 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs @@ -816,10 +816,6 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = } #endif } - else if (RuntimeHelpers.IsReferenceOrContainsReferences()) - { - return AllocateArray(length, pinned: true); - } // kept outside of the small arrays hot path to have inlining without big size growth return AllocateNewUninitializedArray(length, pinned); From 13fe7a16ae5407a1b8b4d91f93a14e6fa2d1714b Mon Sep 17 00:00:00 2001 From: Janus Thorborg Date: Fri, 21 Jul 2023 19:45:06 +0200 Subject: [PATCH 07/15] Simplify pinning paths. All conditional paths in GC.Allocate*Array now handle pinning unconditionally out of the main branch. --- .../System.Private.CoreLib/src/System/GC.CoreCLR.cs | 4 +--- .../System.Private.CoreLib/src/System/GC.NativeAot.cs | 8 +++----- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs index c80de78e4374ab..d82aec4ddaacf1 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs @@ -755,9 +755,7 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = #endif } - GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL; - if (pinned) - flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; + GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL | GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; return Unsafe.As(AllocateNewArray(typeof(T[]).TypeHandle.Value, length, flags)); } diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs index 84cd270681e7c3..589fb7193c6215 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs @@ -818,13 +818,11 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = } // kept outside of the small arrays hot path to have inlining without big size growth - return AllocateNewUninitializedArray(length, pinned); + return AllocateNewUninitializedArray(length); - static T[] AllocateNewUninitializedArray(int length, bool pinned) + static T[] AllocateNewUninitializedArray(int length) { - GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL; - if (pinned) - flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; + GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL | GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; if (length < 0) throw new OverflowException(); From 3aeca6b5fddde93139496cb6be0c834b764e6920 Mon Sep 17 00:00:00 2001 From: Janus Thorborg Date: Fri, 21 Jul 2023 20:01:26 +0200 Subject: [PATCH 08/15] Revert mistake in prior refactor. Preserved conditional pinning flags conditional in !pinned + debug | small size flags. --- .../System.Private.CoreLib/src/System/GC.CoreCLR.cs | 4 +++- .../System.Private.CoreLib/src/System/GC.NativeAot.cs | 8 +++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs index d82aec4ddaacf1..c80de78e4374ab 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs @@ -755,7 +755,9 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = #endif } - GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL | GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; + GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL; + if (pinned) + flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; return Unsafe.As(AllocateNewArray(typeof(T[]).TypeHandle.Value, length, flags)); } diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs index 589fb7193c6215..84cd270681e7c3 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs @@ -818,11 +818,13 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = } // kept outside of the small arrays hot path to have inlining without big size growth - return AllocateNewUninitializedArray(length); + return AllocateNewUninitializedArray(length, pinned); - static T[] AllocateNewUninitializedArray(int length) + static T[] AllocateNewUninitializedArray(int length, bool pinned) { - GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL | GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; + GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL; + if (pinned) + flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; if (length < 0) throw new OverflowException(); From 2e502dc55b5c6c5bfc49086e1b9664aa920d1d61 Mon Sep 17 00:00:00 2001 From: Janus Thorborg Date: Fri, 21 Jul 2023 20:46:25 +0200 Subject: [PATCH 09/15] Renaming in tests. --- src/libraries/System.Runtime/tests/System/GCTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Runtime/tests/System/GCTests.cs b/src/libraries/System.Runtime/tests/System/GCTests.cs index 66f38d4008ef96..58649978b1f9ce 100644 --- a/src/libraries/System.Runtime/tests/System/GCTests.cs +++ b/src/libraries/System.Runtime/tests/System/GCTests.cs @@ -1085,16 +1085,16 @@ void TryType() [InlineData(false), InlineData(true)] private unsafe static void AllocateArrayPinned_ManagedValueType_CanRoundtripThroughPointer(bool uninitialized) { - const int k_Length = 100; + const int length = 100; var rng = new Random(0xAF); - var array = uninitialized ? GC.AllocateUninitializedArray>(k_Length, pinned: true) : GC.AllocateArray>(k_Length, pinned: true); + var array = uninitialized ? GC.AllocateUninitializedArray>(length, pinned: true) : GC.AllocateArray>(length, pinned: true); byte* pointer = (byte*)Unsafe.AsPointer(ref array[0]); var size = Unsafe.SizeOf>(); - for(int i = 0; i < k_Length; ++i) + for(int i = 0; i < length; ++i) { - var idx = rng.Next(k_Length); + var idx = rng.Next(length); ref var evt = ref Unsafe.AsRef>(pointer + size * idx); var stringValue = rng.NextSingle().ToString(); From d7dfad7341d828be78b09cbe1e49e696d09fac4c Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 1 Aug 2023 17:18:09 -0700 Subject: [PATCH 10/15] Update src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs Co-authored-by: jthorborg --- src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs index c80de78e4374ab..8c8a93fed85cd7 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs @@ -759,6 +759,7 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = if (pinned) flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; + // Runtime overrides GC_ALLOC_ZEROING_OPTIONAL if the type contains references, so we don't need to worry about that. return Unsafe.As(AllocateNewArray(typeof(T[]).TypeHandle.Value, length, flags)); } From 3a1bca6f930b46d699bb231ac751224460490c71 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Tue, 1 Aug 2023 17:21:50 -0700 Subject: [PATCH 11/15] Apply suggestions from code review --- src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs index 8c8a93fed85cd7..7d7a0e2263953d 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs @@ -755,11 +755,11 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = #endif } + // Runtime overrides GC_ALLOC_ZEROING_OPTIONAL if the type contains references, so we don't need to worry about that. GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL; if (pinned) flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; - // Runtime overrides GC_ALLOC_ZEROING_OPTIONAL if the type contains references, so we don't need to worry about that. return Unsafe.As(AllocateNewArray(typeof(T[]).TypeHandle.Value, length, flags)); } From 4b805e5f82c8d4d0e64db938e4378c7df4237b0b Mon Sep 17 00:00:00 2001 From: Janus Thorborg Date: Thu, 10 Aug 2023 18:33:14 +0200 Subject: [PATCH 12/15] Don't use `var` if type name doesn't exist explicitly on right-hand side --- src/libraries/System.Runtime/tests/System/GCTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Runtime/tests/System/GCTests.cs b/src/libraries/System.Runtime/tests/System/GCTests.cs index 58649978b1f9ce..3f76fa0d74d41b 100644 --- a/src/libraries/System.Runtime/tests/System/GCTests.cs +++ b/src/libraries/System.Runtime/tests/System/GCTests.cs @@ -1088,16 +1088,16 @@ private unsafe static void AllocateArrayPinned_ManagedValueType_CanRoundtripThro const int length = 100; var rng = new Random(0xAF); - var array = uninitialized ? GC.AllocateUninitializedArray>(length, pinned: true) : GC.AllocateArray>(length, pinned: true); + EmbeddedValueType[] array = uninitialized ? GC.AllocateUninitializedArray>(length, pinned: true) : GC.AllocateArray>(length, pinned: true); byte* pointer = (byte*)Unsafe.AsPointer(ref array[0]); var size = Unsafe.SizeOf>(); for(int i = 0; i < length; ++i) { - var idx = rng.Next(length); - ref var evt = ref Unsafe.AsRef>(pointer + size * idx); + int idx = rng.Next(length); + ref EmbeddedValueType evt = ref Unsafe.AsRef>(pointer + size * idx); - var stringValue = rng.NextSingle().ToString(); + string stringValue = rng.NextSingle().ToString(); evt.Value = stringValue; Assert.Equal(evt.Value, array[idx].Value); From f3aff36df0cb4ca8f5fc562d8093bba37d968094 Mon Sep 17 00:00:00 2001 From: Janus Thorborg Date: Thu, 10 Aug 2023 18:34:18 +0200 Subject: [PATCH 13/15] PR feedback for using terse method tables and JIT intrinsics for GC array allocation that should be slightly faster. --- src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs | 4 ++-- .../System.Private.CoreLib/src/System/GC.NativeAot.cs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs index 7d7a0e2263953d..f25856d438cffe 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs @@ -760,7 +760,7 @@ public static unsafe T[] AllocateUninitializedArray(int length, bool pinned = if (pinned) flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; - return Unsafe.As(AllocateNewArray(typeof(T[]).TypeHandle.Value, length, flags)); + return Unsafe.As(AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(T[]).TypeHandle), length, flags)); } /// @@ -778,7 +778,7 @@ public static T[] AllocateArray(int length, bool pinned = false) // T[] rathe flags = GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP; } - return Unsafe.As(AllocateNewArray(typeof(T[]).TypeHandle.Value, length, flags)); + return Unsafe.As(AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(T[]).TypeHandle), length, flags)); } [MethodImpl(MethodImplOptions.InternalCall)] diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs index 84cd270681e7c3..54934f10be61f8 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs @@ -830,7 +830,7 @@ static T[] AllocateNewUninitializedArray(int length, bool pinned) throw new OverflowException(); T[]? array = null; - RuntimeImports.RhAllocateNewArray(EETypePtr.EETypePtrOf().RawValue, (uint)length, (uint)flags, Unsafe.AsPointer(ref array)); + RuntimeImports.RhAllocateNewArray(MethodTable.Of(), (uint)length, (uint)flags, Unsafe.AsPointer(ref array)); if (array == null) throw new OutOfMemoryException(); @@ -857,7 +857,7 @@ public static unsafe T[] AllocateArray(int length, bool pinned = false) throw new OverflowException(); T[]? array = null; - RuntimeImports.RhAllocateNewArray(EETypePtr.EETypePtrOf().RawValue, (uint)length, (uint)flags, Unsafe.AsPointer(ref array)); + RuntimeImports.RhAllocateNewArray(MethodTable.Of(), (uint)length, (uint)flags, Unsafe.AsPointer(ref array)); if (array == null) throw new OutOfMemoryException(); From 349970a025a343cd73f5dfba95803afa6187b329 Mon Sep 17 00:00:00 2001 From: Janus Thorborg Date: Thu, 10 Aug 2023 18:35:21 +0200 Subject: [PATCH 14/15] Changed EmbeddedValueType and added comment. After a longer discussion, settled on a slightly augmented suggestion that isn't as controversial as the prior one. --- src/libraries/System.Runtime/tests/System/GCTests.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Runtime/tests/System/GCTests.cs b/src/libraries/System.Runtime/tests/System/GCTests.cs index 3f76fa0d74d41b..02f4baa02142fe 100644 --- a/src/libraries/System.Runtime/tests/System/GCTests.cs +++ b/src/libraries/System.Runtime/tests/System/GCTests.cs @@ -1060,8 +1060,15 @@ private static void AllocateArrayTooLarge() struct EmbeddedValueType { - unsafe fixed byte _[7]; + // There's a few extra fields here to ensure any reads and writes to Value reasonably are not just offset 0. + // This is a low-level smoke check that can give a bit of confidence in pointer arithmetic. + // This isn't a guarantee since CoreCLR reorders fields and ignores the implicit sequential consistency of + // structs, but it will never hurt the test. + object _1; + byte _2; public T Value; + int _3; + string _4; } [Theory] From bfe62ec5cdf25039892ca000a1399d286eb9042a Mon Sep 17 00:00:00 2001 From: Janus Thorborg Date: Fri, 11 Aug 2023 15:34:50 +0200 Subject: [PATCH 15/15] Suggestions from PR feedback. * Fixing signature for `RhAllocateNewArray` in NativeAOT to directly use a `MethodTable*` * Adding explicit structlayout to silence warning for EmbeddedValueType in GCTests, and improved the comment. --- .../src/System/Runtime/RuntimeImports.cs | 2 +- src/libraries/System.Runtime/tests/System/GCTests.cs | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs index 6e903753f1a47f..305c5db303df93 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs @@ -239,7 +239,7 @@ internal struct GCHeapHardLimitInfo internal static extern void RhGetMemoryInfo(ref byte info, GCKind kind); [LibraryImport(RuntimeLibrary)] - internal static unsafe partial void RhAllocateNewArray(IntPtr pArrayEEType, uint numElements, uint flags, void* pResult); + internal static unsafe partial void RhAllocateNewArray(MethodTable* pArrayEEType, uint numElements, uint flags, void* pResult); [LibraryImport(RuntimeLibrary)] internal static unsafe partial void RhAllocateNewObject(IntPtr pEEType, uint flags, void* pResult); diff --git a/src/libraries/System.Runtime/tests/System/GCTests.cs b/src/libraries/System.Runtime/tests/System/GCTests.cs index 02f4baa02142fe..d161b3bbdde5bb 100644 --- a/src/libraries/System.Runtime/tests/System/GCTests.cs +++ b/src/libraries/System.Runtime/tests/System/GCTests.cs @@ -1058,12 +1058,13 @@ private static void AllocateArrayTooLarge() Assert.Throws(() => GC.AllocateUninitializedArray(int.MaxValue, pinned: true)); } + [StructLayout(LayoutKind.Sequential)] struct EmbeddedValueType { // There's a few extra fields here to ensure any reads and writes to Value reasonably are not just offset 0. // This is a low-level smoke check that can give a bit of confidence in pointer arithmetic. - // This isn't a guarantee since CoreCLR reorders fields and ignores the implicit sequential consistency of - // structs, but it will never hurt the test. + // The CLR is permitted to reorder fields and ignore the sequential consistency of value types if they contain + // managed references, but it will never hurt the test. object _1; byte _2; public T Value;