Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow reference types for pinned GC.AllocateArray() #89293

Merged
merged 16 commits into from
Aug 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 2 additions & 14 deletions src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs
Original file line number Diff line number Diff line change
Expand Up @@ -732,9 +732,6 @@ internal static void UnregisterMemoryLoadChangeNotification(Action notification)
/// <typeparam name="T">Specifies the type of the array element.</typeparam>
/// <param name="length">Specifies the length of the array.</param>
/// <param name="pinned">Specifies whether the allocated array must be pinned.</param>
/// <remarks>
/// If pinned is set to true, <typeparamref name="T"/> must not be a reference type or a type that contains object references.
/// </remarks>
[MethodImpl(MethodImplOptions.AggressiveInlining)] // forced to ensure no perf drop for small memory buffers (hot path)
public static unsafe T[] AllocateUninitializedArray<T>(int length, bool pinned = false) // T[] rather than T?[] to match `new T[length]` behavior
{
jkotas marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -757,11 +754,8 @@ public static unsafe T[] AllocateUninitializedArray<T>(int length, bool pinned =

#endif
}
else if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
{
ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(T));
}

// 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;
jkotas marked this conversation as resolved.
Show resolved Hide resolved
if (pinned)
flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP;
Expand All @@ -775,22 +769,16 @@ public static unsafe T[] AllocateUninitializedArray<T>(int length, bool pinned =
/// <typeparam name="T">Specifies the type of the array element.</typeparam>
/// <param name="length">Specifies the length of the array.</param>
/// <param name="pinned">Specifies whether the allocated array must be pinned.</param>
/// <remarks>
/// If pinned is set to true, <typeparamref name="T"/> must not be a reference type or a type that contains object references.
/// </remarks>
public static T[] AllocateArray<T>(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<T>())
ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(T));

flags = GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP;
}

return Unsafe.As<T[]>(AllocateNewArray(typeof(T[]).TypeHandle.Value, length, flags));
return Unsafe.As<T[]>(AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(T[]).TypeHandle), length, flags));
}

[MethodImpl(MethodImplOptions.InternalCall)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -795,9 +795,6 @@ internal static ulong GetSegmentSize()
/// <typeparam name="T">Specifies the type of the array element.</typeparam>
/// <param name="length">Specifies the length of the array.</param>
/// <param name="pinned">Specifies whether the allocated array must be pinned.</param>
/// <remarks>
/// If pinned is set to true, <typeparamref name="T"/> must not be a reference type or a type that contains object references.
/// </remarks>
[MethodImpl(MethodImplOptions.AggressiveInlining)] // forced to ensure no perf drop for small memory buffers (hot path)
public static unsafe T[] AllocateUninitializedArray<T>(int length, bool pinned = false)
{
Expand All @@ -819,10 +816,6 @@ public static unsafe T[] AllocateUninitializedArray<T>(int length, bool pinned =
}
#endif
}
else if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
{
ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(T));
}

// kept outside of the small arrays hot path to have inlining without big size growth
return AllocateNewUninitializedArray(length, pinned);
Expand All @@ -837,7 +830,7 @@ static T[] AllocateNewUninitializedArray(int length, bool pinned)
throw new OverflowException();

T[]? array = null;
RuntimeImports.RhAllocateNewArray(EETypePtr.EETypePtrOf<T[]>().RawValue, (uint)length, (uint)flags, Unsafe.AsPointer(ref array));
RuntimeImports.RhAllocateNewArray(MethodTable.Of<T[]>(), (uint)length, (uint)flags, Unsafe.AsPointer(ref array));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please also change the type of RhAllocateNewArray argument to MethodTable* to fix the build break

if (array == null)
throw new OutOfMemoryException();

Expand All @@ -851,26 +844,20 @@ static T[] AllocateNewUninitializedArray(int length, bool pinned)
/// <typeparam name="T">Specifies the type of the array element.</typeparam>
/// <param name="length">Specifies the length of the array.</param>
/// <param name="pinned">Specifies whether the allocated array must be pinned.</param>
/// <remarks>
/// If pinned is set to true, <typeparamref name="T"/> must not be a reference type or a type that contains object references.
/// </remarks>
public static unsafe T[] AllocateArray<T>(int length, bool pinned = false)
{
GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_NO_FLAGS;

if (pinned)
{
if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(T));

flags = GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP;
}

jkotas marked this conversation as resolved.
Show resolved Hide resolved
if (length < 0)
throw new OverflowException();

T[]? array = null;
RuntimeImports.RhAllocateNewArray(EETypePtr.EETypePtrOf<T[]>().RawValue, (uint)length, (uint)flags, Unsafe.AsPointer(ref array));
RuntimeImports.RhAllocateNewArray(MethodTable.Of<T[]>(), (uint)length, (uint)flags, Unsafe.AsPointer(ref array));
if (array == null)
throw new OutOfMemoryException();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
57 changes: 53 additions & 4 deletions src/libraries/System.Runtime/tests/System/GCTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1058,11 +1058,60 @@ private static void AllocateArrayTooLarge()
Assert.Throws<OutOfMemoryException>(() => GC.AllocateUninitializedArray<double>(int.MaxValue, pinned: true));
}

[Fact]
private static void AllocateArrayRefType()
[StructLayout(LayoutKind.Sequential)]
struct EmbeddedValueType<T>
jkotas marked this conversation as resolved.
Show resolved Hide resolved
{
// 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.
// 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;
int _3;
string _4;
}

[Theory]
[InlineData(false), InlineData(true)]
private static void AllocateArray_UninitializedOrNot_WithManagedType_DoesNotThrow(bool pinned)
{
void TryType<T>()
{
GC.AllocateUninitializedArray<T>(100, pinned);
GC.AllocateArray<T>(100, pinned);

GC.AllocateArray<EmbeddedValueType<T>>(100, pinned);
GC.AllocateUninitializedArray<EmbeddedValueType<T>>(100, pinned);
}

TryType<string>();
TryType<object>();
}

[Theory]
[InlineData(false), InlineData(true)]
private unsafe static void AllocateArrayPinned_ManagedValueType_CanRoundtripThroughPointer(bool uninitialized)
{
GC.AllocateUninitializedArray<string>(100);
Assert.Throws<ArgumentException>(() => GC.AllocateUninitializedArray<string>(100, pinned: true));
const int length = 100;
var rng = new Random(0xAF);

EmbeddedValueType<string>[] array = uninitialized ? GC.AllocateUninitializedArray<EmbeddedValueType<string>>(length, pinned: true) : GC.AllocateArray<EmbeddedValueType<string>>(length, pinned: true);
byte* pointer = (byte*)Unsafe.AsPointer(ref array[0]);
var size = Unsafe.SizeOf<EmbeddedValueType<string>>();

for(int i = 0; i < length; ++i)
{
int idx = rng.Next(length);
ref EmbeddedValueType<string> evt = ref Unsafe.AsRef<EmbeddedValueType<string>>(pointer + size * idx);

string stringValue = rng.NextSingle().ToString();
evt.Value = stringValue;

Assert.Equal(evt.Value, array[idx].Value);
}

GC.KeepAlive(array);
}

[Fact]
Expand Down
2 changes: 0 additions & 2 deletions src/mono/System.Private.CoreLib/src/System/GC.Mono.cs
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,6 @@ public static T[] AllocateUninitializedArray<T>(int length, bool pinned = false)
public static T[] AllocateArray<T>(int length, bool pinned = false)
{
if (pinned) {
if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(T));
return Unsafe.As<T[]>(AllocPinnedArray(typeof(T[]), length));
}

Expand Down
Loading