Adding Memory.Pin() to eventually replace Memory.Retain(bool) #17269
Conversation
|
@ahsonkhan let this change propagate through all of ASP.NET before the removal. |
OK. How can I help to get it propagated? Let me know when there will be a dependencies update in ASP.NET. |
We're working on maestro bot integration aspnet/Universe#968 |
@davidfowl can you push to get the aspnet maestro stuff turned on today? @ahsonkhan should have the API additions in today; and we'd like to merge the removal PR tomorrow... |
| /// Returns a handle for the array. | ||
| /// <param name="pin">If pin is true, the GC will not move the array and hence its address can be taken</param> | ||
| /// Creates a handle for the memory. | ||
| /// The GC will not move the array until the returned <see cref="MemoryHandle"/> |
There was a problem hiding this comment.
The doc comment should not talk about arrays. In the OwnedMemory more there might not be an array.
There was a problem hiding this comment.
Agreed. The docs should also mention the possibility of GCHandle.Alloc throwing (An instance with nonprimitive (non-blittable) members cannot be pinned.).
I will address this separately after all the changes are in.
ReadOnlyMemory<object> memory = new object[10];
memory.Pin(); // throws ArgumentException| /// </summary> | ||
| public unsafe MemoryHandle Pin() |
There was a problem hiding this comment.
Do we prefer the whole method to be unsafe, or just the part that does pointer arithmetic?
There was a problem hiding this comment.
I kept the changes from Retain minimal. Also, we would end up with two unsafe blocks if we want to make this change (within each condition), which doesn't buy us much. Otherwise, it would be around the entire if block, which is essentially equivalent to it being around the whole method.
|
@ahsonkhan It appears this change has caused 3 regressions in Windows ARM: The failures started with job 1236 here: https://ci.dot.net/job/dotnet_coreclr/job/master/job/arm_cross_checked_windows_nt_flow/, which corresponds to your change. |
Are you sure this is the cause? This PR only added an API, which isn't called by anything in coreclr. Why would that cause a regression? |
|
Ah, looks like it was probably #17161. The ARM builds share x86 tests builds, and both used this PR. |
Related PR in corefx: dotnet/corefx#28517
cc @KrzysztofCwalina, @jkotas, @davidfowl, @stephentoub, @dotnet/corefxlab-contrib