Add CreateFromPinnedArray to System.Memory ref and add tests by ahsonkhan · Pull Request #28992 · dotnet/corefx

ahsonkhan · 2018-04-10T20:47:16Z

Part of https://github.com/dotnet/corefx/issues/28954

TODO: Update newly added tests to use MemoryMarshal.CreateFromPinnedArray

cc @KrzysztofCwalina, @stephentoub, @GrabYourPitchforks, @davidfowl, @pakrym

ahsonkhan · 2018-04-10T21:27:55Z

+            GC.Collect(2);
+
+            Assert.Equal((int)pinnedPtr, (int)Unsafe.AsPointer(ref MemoryMarshal.GetReference(pinnedMemory.Span)));
+            Assert.NotEqual((int)ptr, (int)Unsafe.AsPointer(ref MemoryMarshal.GetReference(memory.Span)));


Is this test appropriate (i.e. it will always pass due to the GC.Collect)? cc @jkotas

As @KrzysztofCwalina said.

KrzysztofCwalina · 2018-04-10T21:30:03Z

+            int[] pinnedArray = { 90, 91, 92, 93, 94, 95, 96, 97, 98 };
+            GCHandle handle = GCHandle.Alloc(pinnedArray, GCHandleType.Pinned);
+
+            var memory = new Memory<int>(unpinnedArray, 0, 2);


Can you call it unpinnedMemory?

No longer relevant.

KrzysztofCwalina · 2018-04-10T21:30:29Z

+        {
+            int[] unpinnedArray = { 90, 91, 92, 93, 94, 95, 96, 97, 98 };
+            int[] pinnedArray = { 90, 91, 92, 93, 94, 95, 96, 97, 98 };
+            GCHandle handle = GCHandle.Alloc(pinnedArray, GCHandleType.Pinned);


Can you call it pinnedHandle?

KrzysztofCwalina · 2018-04-10T21:33:01Z

+            void* memoryHandlePinnedPtr = pinnedMemory.Pin().Pointer;
+
+            GC.Collect();
+            GC.Collect(2);


I don't think this is absolutely guaranteeing that the GC will move the array, and so the test might fail from time to time.

That's what I thought. In which case, I will remove this Assert (but I will keep the "pinned" components).

KrzysztofCwalina · 2018-04-10T21:34:22Z

+            Assert.NotEqual((int)ptr, (int)Unsafe.AsPointer(ref MemoryMarshal.GetReference(memory.Span)));
+
+            Assert.Equal((int)memoryHandlePinnedPtr, (int)handle.AddrOfPinnedObject().ToPointer());
+            Assert.NotEqual((int)memoryHandlePtr, (int)memory.Pin().Pointer);


Would it make sense to unpin the arrays (both of them) after the test?

Do we need to explicitly call Free on the GCHandle? It won't automatically get disposed after the method returns?

It looks like we do:

This GCHandle must be released with Free when it is no longer needed.

KrzysztofCwalina · 2018-04-10T21:37:02Z

+            Assert.NotEqual((int)memoryHandlePtr, (int)memory.Pin().Pointer);
+        }
+    }
+}


It would be good to have a test that verifies that when memory is created from a prepinned array, it does not cause additional GCHandles to be created during Memory.Pin calls. I am not sure how to write such test, but maybe somebody on the runtime team would know. @jkotas?

E.g. you would need to start the test in separate process, enable tracing for it, and verify via trace that there was no GC handle allocated. I do not think it is worth the effort with what we have available today. Writing similar tests may become more viable once we have the ability for processes to self-trace.

Writing similar tests may become more viable once we have the ability for processes to self-trace.

FWIW, we do something similar in a variety of corefx test suites for EventSource-based tracing, e.g.
https://github.com/dotnet/corefx/blob/master/src/System.Net.Security/tests/FunctionalTests/LoggingTest.cs#L31
but assuming GC handle-related tracing doesn't come from an EventSource, as Jan suggests that's going to be a lot more involved.

…C will move it.

stephentoub · 2018-04-10T22:33:10Z

        public static System.Memory<T> AsMemory<T>(System.ReadOnlyMemory<T> memory) { throw null; }
        public static System.ReadOnlySpan<TTo> Cast<TFrom, TTo>(System.ReadOnlySpan<TFrom> span) where TFrom : struct where TTo : struct { throw null; }
        public static System.Span<TTo> Cast<TFrom, TTo>(System.Span<TFrom> span) where TFrom : struct where TTo : struct { throw null; }
+        public static System.Memory<T> CreateFromPinnedArray<T>(T[] array, int start, int length) { throw null; }


You're not also removing it from Memory? Are you doing that separately?

I was thinking of staging the change and removing it after asp.net updates its use. Depending on how quickly they can consume new corefx packages, I might remove it earlier. dotnet/extensions#335

I wouldn't block (stage) the change this close to the RC deadline. They can make the update as part of the dotnet-maestro-bot PR. /c @pakrym

* Add CreateFromPinnedArray to MemoryMarshal * Address PR feedback: Add a warning remark to CreateFromPinnedArray Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

…eateMemory

ahsonkhan · 2018-04-11T07:38:08Z

Cherry-picked from mirror PR: #29012

ahsonkhan · 2018-04-11T17:57:36Z

+                if (_length < 0)
+                {
 #if FEATURE_PORTABLE_SPAN
-                void* pointer = Unsafe.Add<T>((void*)handle.AddrOfPinnedObject(), _index);


I want to highlight a fix for a bug here in Memory.Pin where handle could be null (for portable span, for pre-pinned arrays). Please review.

Tests from this PR caught the bug:
https://mc.dot.net/#/user/ahsonkhan/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/f4b9ba1fd81623c838cc27647936c3569e83aa22/workItem/System.Memory.Tests/analysis/xunit/System.SpanTests.MemoryMarshalTests~2FCreateFromPinnedArrayVerifyPinning
System.SpanTests.MemoryMarshalTests/CreateFromPinnedArrayVerifyPinning

System.InvalidOperationException : Handle is not initialized. Stack Trace : at System.Runtime.InteropServices.GCHandle.AddrOfPinnedObject() at System.Memory`1.Pin() in D:\j\workspace\windows-TGrou---2a8f9c29\src\Common\src\CoreLib\System\Memory.cs:line 358 at System.SpanTests.MemoryMarshalTests.CreateFromPinnedArrayVerifyPinning() in D:\j\workspace\windows-TGrou---2a8f9c29\src\System.Memory\tests\MemoryMarshal\CreateFromPinnedArray.cs:line 140

cc @stephentoub, @KrzysztofCwalina, @jkotas, @GrabYourPitchforks

ahsonkhan · 2018-04-11T18:51:09Z

@dotnet-bot test OSX x64 Debug Build

https://github.com/dotnet/corefx/issues/29036

…corefx#28992) * Add CreateFromPinnedArray to System.Memory ref and add tests * Update CreateFromPinnedArray tests * Removing asserts on unpinned array since there is no guarantee that GC will move it. * Add CreateFromPinnedArray to MemoryMarshal (dotnet/coreclrdotnet/corefx#17500) * Add CreateFromPinnedArray to MemoryMarshal * Address PR feedback: Add a warning remark to CreateFromPinnedArray Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com> * Fixing bug in Memory.Pin and adding API to uapaot baseline Commit migrated from dotnet/corefx@b4d701a

Add CreateFromPinnedArray to System.Memory ref and add tests

f5a81a9

ahsonkhan added the area-System.Memory label Apr 10, 2018

ahsonkhan self-assigned this Apr 10, 2018

ahsonkhan mentioned this pull request Apr 10, 2018

Add CreateFromPinnedArray to MemoryMarshal dotnet/coreclr#17500

Merged

Update CreateFromPinnedArray tests

51de41c

ahsonkhan commented Apr 10, 2018

View reviewed changes

KrzysztofCwalina reviewed Apr 10, 2018

View reviewed changes

Removing asserts on unpinned array since there is no guarantee that G…

47ceb27

…C will move it.

ahsonkhan added the blocked Issue/PR is blocked on something - see comments label Apr 10, 2018

ahsonkhan mentioned this pull request Apr 10, 2018

Reacting to changes to CreateFromPinnedArray dotnet/extensions#335

Merged

stephentoub reviewed Apr 10, 2018

View reviewed changes

Add CreateFromPinnedArray to MemoryMarshal (dotnet/coreclr#17500)

84dfa18

* Add CreateFromPinnedArray to MemoryMarshal * Address PR feedback: Add a warning remark to CreateFromPinnedArray Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

ahsonkhan removed the blocked Issue/PR is blocked on something - see comments label Apr 11, 2018

Merge branch 'master' of https://github.com/dotnet/corefx into MoveCr…

f4b9ba1

…eateMemory

ahsonkhan mentioned this pull request Apr 11, 2018

Mirror changes from dotnet/coreclr #29012

Closed

Fixing bug in Memory.Pin and adding API to uapaot baseline

95b8f31

ahsonkhan commented Apr 11, 2018

View reviewed changes

jkotas approved these changes Apr 11, 2018

View reviewed changes

joshfree approved these changes Apr 11, 2018

View reviewed changes

joshfree added this to the 2.1.0 milestone Apr 11, 2018

stephentoub modified the milestone: 2.1.0 Apr 11, 2018

ahsonkhan merged commit b4d701a into dotnet:master Apr 11, 2018

ahsonkhan deleted the MoveCreateMemory branch April 11, 2018 20:43

ahsonkhan mentioned this pull request Apr 12, 2018

Remove Memory.CreateFromPinnedArray from the ref assemblies #29077

Closed

ahsonkhan mentioned this pull request Apr 20, 2018

Mirror changes from dotnet/corefx dotnet/coreclr#17709

Merged

Conversation

ahsonkhan commented Apr 10, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KrzysztofCwalina Apr 10, 2018 • 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!

ahsonkhan Apr 10, 2018 • 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!

KrzysztofCwalina Apr 10, 2018 • 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!

ahsonkhan Apr 10, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan commented Apr 11, 2018

Uh oh!

ahsonkhan Apr 11, 2018 • 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!

ahsonkhan commented Apr 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ahsonkhan commented Apr 10, 2018 •

edited

Loading

KrzysztofCwalina Apr 10, 2018 •

edited

Loading

ahsonkhan Apr 10, 2018 •

edited

Loading

KrzysztofCwalina Apr 10, 2018 •

edited

Loading

ahsonkhan Apr 10, 2018 •

edited

Loading

ahsonkhan Apr 11, 2018 •

edited

Loading