Adding public API for Pinned Object Heap allocations by VSadov · Pull Request #33526 · dotnet/runtime (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation42 Commits13 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Exposing the following public API:
System.GC : public static T[] AllocateUninitializedArray(int length, bool pinned = false); public static T[] AllocateArray(int length, bool pinned = false);
Did these APIs go through API review?
@AaronRobinsonMSFT - yes, twice. Once - long time ago. And again in november, to make sure and confirm.
[WIP] Adding API for POH allocations Adding public API for Pinned Object Heap allocations
I think the change is ready for review. Please take a look.
| /// otherwise it's not pinned. |
|---|
| /// pinned requires that T does not contain object references. |
| /// |
| public static T[] AllocateArray(int length, bool pinned = false) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be better to have this as AllocatePinnedArray<T> only? What is the difference between new object[10] and AllocateArray<object>(10) ?
Choose a reason for hiding this comment
The 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.
Choose a reason for hiding this comment
The 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.
Choose a reason for hiding this comment
The 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?
Choose a reason for hiding this comment
The 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...
Choose a reason for hiding this comment
The 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 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.
@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?
| } |
|---|
| #endif |
| // kept outside of the small arrays hot path to have inlining without big size growth |
Choose a reason for hiding this comment
The 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?
Choose a reason for hiding this comment
The 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 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)
| @@ -653,29 +661,63 @@ internal static void UnregisterMemoryLoadChangeNotification(Action notification) |
|---|
| /// |
| /// 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. |
| /// |
| [MethodImpl(MethodImplOptions.AggressiveInlining)] // forced to ensure no perf drop for small memory buffers (hot path) |
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?
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)
@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.
Any more comments on this?
since you make the AllocateUninitializedArray public with this PR, please remember to have a doc PR to document it.
Yes. Need to update docs. I've entered a tracking issue for that - #33685
| @@ -149,33 +149,17 @@ class Object |
|---|
| m_pMethTab = pMT; |
| } |
| VOID SetMethodTable(MethodTable *pMT |
| DEBUG_ARG(BOOL bAllowArray = FALSE)) |
| VOID SetMethodTable(MethodTable *pMT) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VOID SetMethodTable(MethodTable *pMT) [](start = 4, length = 37)
why get rid of the debug arg? its existence seems very intentional.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see; makes sense
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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.
formatting job was failing due to #33693
This pull request was closed.
Reviewers
