Adding public API for Pinned Object Heap allocations #33526

VSadov · 2020-03-12T18:43:25Z

Exposing the following public API:

System.GC :
        public static T[] AllocateUninitializedArray<T>(int length, bool pinned = false);
        public static T[] AllocateArray<T>(int length, bool pinned = false);

AaronRobinsonMSFT · 2020-03-14T03:22:07Z

Did these APIs go through API review?

VSadov · 2020-03-14T03:52:48Z

@AaronRobinsonMSFT - yes, twice. Once - long time ago. And again in november, to make sure and confirm.

VSadov · 2020-03-15T05:14:01Z

I think the change is ready for review. Please take a look.

src/mono/netcore/System.Private.CoreLib/src/System/GC.Mono.cs

src/libraries/System.Runtime/tests/System/GCTests.cs

marek-safar · 2020-03-15T06:46:59Z

src/coreclr/src/System.Private.CoreLib/src/System/GC.cs

+        /// otherwise it's not pinned.
+        /// pinned requires that T does not contain object references.
+        /// </summary>
+        public static T[] AllocateArray<T>(int length, bool pinned = false)


Wouldn't be better to have this as AllocatePinnedArray<T> only? What is the difference between new object[10] and AllocateArray<object>(10) ?

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.

It seems odd to me that every time someone uses this API one has to type the explicit parameter to make the code readable.

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?

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...

For the pool - I think from the usability point, it might be preferable to just add GetPinned to the current pool API.
Alternatively we could have a separate pool for pinned arrays, but then it puts a burden on the user to remember where to return.

src/mono/netcore/System.Private.CoreLib/src/System/GC.Mono.cs

stephentoub · 2020-03-16T02:29:10Z

@VSadov, can you link to the relevant API review issue? I see #27146 (comment), which has a different shape than is implemented here. Is that comment out-of-date and there was a newer discussion?

stephentoub · 2020-03-16T02:31:59Z

src/coreclr/src/System.Private.CoreLib/src/System/GC.cs

            }
-#endif
+
            // kept outside of the small arrays hot path to have inlining without big size growth


Should the above else if block then also be moved into the AllocateNewUninitializedArray method, to keep it out of the inlinling path?

The shape of this method was tweaked to make the case when we fallback to T[] fast. That is only relevant if pinned == false. (example: look at use in StringBuilder).
I expect that this entire block will be omitted in such case. That is assuming the method does get inlined, which it should.

In reply to: 392757230 [](ancestors = 392757230)

src/coreclr/src/System.Private.CoreLib/src/System/GC.cs

stephentoub · 2020-03-16T02:40:53Z

src/coreclr/src/System.Private.CoreLib/src/System/GC.cs

@@ -653,29 +661,63 @@ 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.
+        /// pinned: true means you want to pin this object that you are allocating
+        /// otherwise it's not pinned.
+        /// pinned requires that T does not contain object references.
        /// </summary>
        [MethodImpl(MethodImplOptions.AggressiveInlining)] // forced to ensure no perf drop for small memory buffers (hot path)


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?

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)

VSadov · 2020-03-16T17:05:38Z

@stephentoub - the goal is to have API as in: #27146 (comment) .

The main difference is that we are not doing all the features/parameters at once.
As more features are added we will be adding new overloads and making parameters in previous version not optional - for compat/versioning reasons.

VSadov · 2020-03-16T22:36:04Z

Refactored AllocateArray as @stephentoub suggested

VSadov · 2020-03-17T17:14:22Z

Any more comments on this?

Maoni0 · 2020-03-17T19:31:23Z

since you make the AllocateUninitializedArray public with this PR, please remember to have a doc PR to document it.

VSadov · 2020-03-17T19:35:18Z

Yes. Need to update docs. I've entered a tracking issue for that - #33685

Maoni0 · 2020-03-17T20:33:21Z

src/coreclr/src/vm/object.h

@@ -149,33 +149,17 @@ class Object
        m_pMethTab = pMT;
    }

-    VOID SetMethodTable(MethodTable *pMT
-                        DEBUG_ARG(BOOL bAllowArray = FALSE))
+    VOID SetMethodTable(MethodTable *pMT)


VOID SetMethodTable(MethodTable *pMT) [](start = 4, length = 37)

why get rid of the debug arg? its existence seems very intentional.

This looks like a remnant from the times when array's method tables could be shared for arrays of reference type. It was important to not call this API by accident.
There is no sharing any more and we use the same API, regardless of the type of the object and whether it is an array or not. The check was still there, but not validating or protecting anything.

I see; makes sense

Maoni0

LGTM!

VSadov · 2020-03-18T02:24:08Z

Thanks!!

…lloc.

ghost · 2020-03-18T02:37:16Z

Hello @VSadov!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

VSadov · 2020-03-18T02:38:07Z

formatting job was failing due to #33693

Dotnet-GitSync-Bot added the area-System.Runtime label Mar 12, 2020

VSadov mentioned this pull request Mar 14, 2020

Implement pinned flavors of GC.Allocate[Uninitialized]Array on Mono #33583

Closed

VSadov force-pushed the pohApi branch from 7deecdf to 30443a5 Compare March 14, 2020 03:15

VSadov force-pushed the pohApi branch from 30443a5 to 32522d5 Compare March 15, 2020 01:23

VSadov changed the title ~~[WIP] Adding API for POH allocations~~ Adding public API for Pinned Object Heap allocations Mar 15, 2020

VSadov requested review from Maoni0 and jkotas March 15, 2020 05:11

VSadov marked this pull request as ready for review March 15, 2020 05:11

VSadov requested review from akoeplinger, EgorBo, marek-safar, steveisok and vargaz as code owners March 15, 2020 05:12

jkotas reviewed Mar 15, 2020

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/System/GC.Mono.cs Outdated Show resolved Hide resolved

src/libraries/System.Runtime/tests/System/GCTests.cs Show resolved Hide resolved

marek-safar reviewed Mar 15, 2020

View reviewed changes

stephentoub reviewed Mar 16, 2020

View reviewed changes

src/coreclr/src/System.Private.CoreLib/src/System/GC.cs Show resolved Hide resolved

stephentoub reviewed Mar 16, 2020

View reviewed changes

jkotas approved these changes Mar 17, 2020

View reviewed changes

stephentoub approved these changes Mar 17, 2020

View reviewed changes

Maoni0 reviewed Mar 17, 2020

View reviewed changes

Maoni0 approved these changes Mar 18, 2020

View reviewed changes

VSadov added the auto-merge label Mar 18, 2020

VSadov removed the auto-merge label Mar 18, 2020

VSadov added 13 commits March 17, 2020 19:34

Adding API for POH allocations and propagating flags all the way to A…

068f01b

…lloc.

make AllocateUninitializedArray and AllocateArray public

9716f20

Added NYI implementations to Mono

e0ea630

moved tests to libraries

365ff76

Actually use POH and more tests.

14db4c2

Disable tests for the new API on mono

9e59071

mop up remaining TODOs

1fe2678

Fix build breaking whitespace.

293cf33

Mono tabs and mark heavier tests as [Outerloop]

5426565

Mono space before openning parens and braces

8cab8be

Refactored AllocateArray

4c0ddb8

PR feedback

8574dc3

XML Doc comments

523940c

VSadov force-pushed the pohApi branch from 1bc9418 to 523940c Compare March 18, 2020 02:35

dotnet deleted a comment Mar 18, 2020

VSadov added the auto-merge label Mar 18, 2020

ghost merged commit c0ddd1c into dotnet:master Mar 18, 2020

VSadov deleted the pohApi branch March 18, 2020 05:01

VSadov mentioned this pull request Mar 25, 2020

[Pinned Object Heap] update documentation for the new public APIs #33685

Closed

halter73 mentioned this pull request Apr 9, 2020

Revert "Use POH for dummyreadbuffers" tmds/Tmds.LinuxAsync#85

Merged

azhmur mentioned this pull request May 3, 2020

Consider multitargeting .net 5 Drawaes/TrimDB#7

Open

VSadov mentioned this pull request May 16, 2020

Introducing Pinned Object Heap #32283

Merged

benaadams mentioned this pull request Jul 30, 2020

a new GC API for large array allocation #27146

Open

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

This pull request was closed.

Adding public API for Pinned Object Heap allocations #33526

Adding public API for Pinned Object Heap allocations #33526

Uh oh!

Conversation

VSadov commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronRobinsonMSFT commented Mar 14, 2020

Uh oh!

VSadov commented Mar 14, 2020

Uh oh!

VSadov commented Mar 15, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov Mar 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stephentoub commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov commented Mar 16, 2020

Uh oh!

VSadov commented Mar 16, 2020

Uh oh!

VSadov commented Mar 17, 2020

Uh oh!

Maoni0 commented Mar 17, 2020

Uh oh!

VSadov commented Mar 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maoni0 left a comment

Choose a reason for hiding this comment

Uh oh!

VSadov commented Mar 18, 2020

Uh oh!

ghost commented Mar 18, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

VSadov commented Mar 18, 2020

Uh oh!

Uh oh!

VSadov commented Mar 12, 2020 •

edited

Loading

VSadov Mar 15, 2020 •

edited

Loading

stephentoub commented Mar 16, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.