-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Adding public API for Pinned Object Heap allocations #33526
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
Changes from all commits
068f01b
9716f20
e0ea630
365ff76
14db4c2
9e59071
1fe2678
293cf33
5426565
8cab8be
4c0ddb8
8574dc3
523940c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,8 +84,16 @@ public static GCMemoryInfo GetGCMemoryInfo() | |
[DllImport(RuntimeHelpers.QCall, CharSet = CharSet.Unicode)] | ||
internal static extern int _EndNoGCRegion(); | ||
|
||
// keep in sync with GC_ALLOC_FLAGS in gcinterface.h | ||
internal enum GC_ALLOC_FLAGS | ||
{ | ||
GC_ALLOC_NO_FLAGS = 0, | ||
GC_ALLOC_ZEROING_OPTIONAL = 16, | ||
GC_ALLOC_PINNED_OBJECT_HEAP = 64, | ||
}; | ||
|
||
[MethodImpl(MethodImplOptions.InternalCall)] | ||
internal static extern Array AllocateNewArray(IntPtr typeHandle, int length, bool zeroingOptional); | ||
internal static extern Array AllocateNewArray(IntPtr typeHandle, int length, GC_ALLOC_FLAGS flags); | ||
|
||
[MethodImpl(MethodImplOptions.InternalCall)] | ||
private static extern int GetGenerationWR(IntPtr handle); | ||
|
@@ -651,31 +659,74 @@ internal static void UnregisterMemoryLoadChangeNotification(Action notification) | |
} | ||
|
||
/// <summary> | ||
/// Skips zero-initialization of the array if possible. | ||
/// If T contains object references, the array is always zero-initialized. | ||
/// Allocate an array while skipping zero-initialization if possible. | ||
/// </summary> | ||
/// <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) | ||
internal static T[] AllocateUninitializedArray<T>(int length) | ||
public static T[] AllocateUninitializedArray<T>(int length, bool pinned = false) | ||
{ | ||
if (RuntimeHelpers.IsReferenceOrContainsReferences<T>()) | ||
if (!pinned) | ||
{ | ||
return new T[length]; | ||
} | ||
if (RuntimeHelpers.IsReferenceOrContainsReferences<T>()) | ||
{ | ||
return new T[length]; | ||
} | ||
|
||
// for debug builds we always want to call AllocateNewArray to detect AllocateNewArray bugs | ||
// for debug builds we always want to call AllocateNewArray to detect AllocateNewArray bugs | ||
#if !DEBUG | ||
// small arrays are allocated using `new[]` as that is generally faster. | ||
if (length < 2048 / Unsafe.SizeOf<T>()) | ||
// small arrays are allocated using `new[]` as that is generally faster. | ||
if (length < 2048 / Unsafe.SizeOf<T>()) | ||
{ | ||
return new T[length]; | ||
} | ||
#endif | ||
} | ||
else if (RuntimeHelpers.IsReferenceOrContainsReferences<T>()) | ||
{ | ||
return new T[length]; | ||
ThrowHelper.ThrowInvalidTypeWithPointersNotSupported(typeof(T)); | ||
} | ||
#endif | ||
|
||
// kept outside of the small arrays hot path to have inlining without big size growth | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the above else if block then also be moved into the AllocateNewUninitializedArray method, to keep it out of the inlinling path? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The shape of this method was tweaked to make the case when we fallback to In reply to: 392757230 [](ancestors = 392757230) |
||
return AllocateNewUninitializedArray(length); | ||
return AllocateNewUninitializedArray(length, pinned); | ||
|
||
// remove the local function when https://github.com/dotnet/coreclr/issues/5329 is implemented | ||
static T[] AllocateNewUninitializedArray(int length) | ||
=> Unsafe.As<T[]>(AllocateNewArray(typeof(T[]).TypeHandle.Value, length, zeroingOptional: true)); | ||
static T[] AllocateNewUninitializedArray(int length, bool pinned) | ||
{ | ||
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<T[]>(AllocateNewArray(typeof(T[]).TypeHandle.Value, length, flags)); | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Allocate an array. | ||
/// </summary> | ||
/// <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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't be better to have this as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are plans to add more overloads to this API, possibly to control target generation or alignment. Indeed, both times when this was reviewed, it was noticed that the trivial case is the same as 'new T[]`, but it seems to be ok and happens with other APIs that have optional parameters. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems odd to me that every time someone uses this API one has to type the explicit parameter to make the code readable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I expect that this API is going to be used very rarely. The more useful API build on top this is going to be a pool for the pinned arrays. The API for that was not designed yet. @VSadov Is it in your plans - what do you expect the pool API to be? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Arguments for going it this way were convincing. When there are more arguments, a new overload will be introduced with 2 optional parameters, while this one becomes not optional and so on... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the pool - I think from the usability point, it might be preferable to just add |
||
{ | ||
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)); | ||
stephentoub marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the additional checks around the pinned argument get removed when the caller uses a const value at the call site / the default argument value, or might the optional parameter end up impacting the perf of existing call sites?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if
AggressiveInlining
works, then the pinned should be const-propagated and related blocks omitted. I did not check in debugger, but this seems a very easy case.In reply to: 392758813 [](ancestors = 392758813)