Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
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.
  • Loading branch information
jthorborg committed Jul 21, 2023
commit ea6de358faf7284b38da47acf1439793cd2e4fce
5 changes: 1 addition & 4 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
{
Expand All @@ -759,7 +756,7 @@ public static unsafe T[] AllocateUninitializedArray<T>(int length, bool pinned =
}
else if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
{
ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(T));
return AllocateArray<T>(length, pinned: true);
}

GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL;
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 @@ -821,7 +818,7 @@ public static unsafe T[] AllocateUninitializedArray<T>(int length, bool pinned =
}
else if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
{
ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(T));
return AllocateArray<T>(length, pinned: true);
}

// kept outside of the small arrays hot path to have inlining without big size growth
Expand Down
28 changes: 12 additions & 16 deletions src/libraries/System.Runtime/tests/System/GCTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1058,41 +1058,37 @@ private static void AllocateArrayTooLarge()
Assert.Throws<OutOfMemoryException>(() => GC.AllocateUninitializedArray<double>(int.MaxValue, pinned: true));
}

[Fact]
private static void AllocateArrayUninitializedPinned_RefType_ThrowsArgumentException()
{
GC.AllocateUninitializedArray<string>(100);
Assert.Throws<ArgumentException>(() => GC.AllocateUninitializedArray<string>(100, pinned: true));
}

struct EmbeddedValueType<T>
{
unsafe fixed byte _[7];
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason for this? It looks like trying to fiddle with offsets and alignment. I tend to avoid these kind of mechanisms in tests without a comment as to the why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a simple way of increasing the size of the struct, making it less trivially laid out and requiring padding bits - to add a bit of confidence in we're not just overwriting the first set of bytes in a memory location.

We could add a comment or just remove it, it's not required.

Copy link
Member

Choose a reason for hiding this comment

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

I think a non-generic struct like this would be sufficient for what you are trying to test here

private struct StructType
{
    internal object _;
    internal string _value;
    internal int _;
}

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<T>()
{
GC.AllocateArray<T>(100);
GC.AllocateArray<T>(100, pinned: true);
GC.AllocateUninitializedArray<T>(100, pinned);
GC.AllocateArray<T>(100, pinned);

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

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

[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<EmbeddedValueType<string>>(k_Length, pinned: true);
var array = uninitialized ? GC.AllocateUninitializedArray<EmbeddedValueType<string>>(k_Length, pinned: true) : GC.AllocateArray<EmbeddedValueType<string>>(k_Length, pinned: true);
byte* pointer = (byte*)Unsafe.AsPointer(ref array[0]);
var size = Unsafe.SizeOf<EmbeddedValueType<string>>();

Expand Down