Implement NativeMemory #54006

tannergooding · 2021-06-10T16:05:01Z

This resolves #33244

dotnet-issue-labeler · 2021-06-10T16:05:06Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

src/coreclr/vm/nativememorynative.cpp

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.cs

tannergooding · 2021-06-10T16:07:56Z

CC. @GrabYourPitchforks, @jkotas

I've put this up to validate it builds/passes on all architectures, but it should be generally in the "right" shape (barring any feedback).

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.cs

src/coreclr/vm/corelib.cpp

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MemAlloc.cs

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.Unix.cs

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.Windows.cs

src/libraries/Native/Unix/System.Native/pal_memory.c

src/libraries/Common/src/Interop/Windows/Ucrtbase/Interop.MemAlloc.cs

src/libraries/Native/Unix/System.Native/pal_memory.c

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.Unix.cs

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.Unix.cs

...ies/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/NativeMemoryTests.cs

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.Unix.cs

tannergooding · 2021-06-15T20:54:09Z

@jkotas, any other feedback here. If not, I think this is "good" and ready for taking the open questions (like renaming Calloc to AllocZeroed) back to API review.

src/libraries/Native/Unix/System.Native/config.h.in

tannergooding · 2021-06-16T19:55:50Z

CI job is here: https://dev.azure.com/dnceng/public/_build/results?buildId=1191094&view=results (it is running now, the push notifications are backed up however)

src/libraries/Native/Unix/System.Native/pal_memory.c

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.Unix.cs

…pServices/NativeMemory.Unix.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>

stephentoub

Thanks.

jkotas · 2021-06-16T21:30:15Z

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.Unix.cs

+
+            if (ptr != null)
+            {
+                Buffer.Memmove(ref *(byte*)newPtr, ref *(byte*)ptr, byteCount);


I do not think this works. byteCount is the new size, but we can only copy old size (the rest should be left uninitialized).

Ah right. I'll need to either expose malloc_usable_size/malloc_size or make this a P/Invoke to a wrapper SystemNative.AlignedRealloc

Updated to be a P/Invoke to a SystemNative_AlignedRealloc to avoid needing 3-4 P/Invoke transitions.

It now correctly does:

Allocate new space

If allocation was successful:

Get usable size of previous allocation

Copy min of old/new size from old to new

Free old allocation

Return result

I've also added a test that explicitly tests going from a small to large allocation and that the bytes are as expected.

…of the last allocation

… paths

…s compile

tannergooding · 2021-06-17T17:57:12Z

Turns out posix_memalign can differ from aligned_alloc specifically for size == 0 (its considered invalid for the latter but may return a valid sizeless allocation for the former) and so the overflow isn't "harmless" for those platforms.

I've fixed it to track the adjustedByteCount and handle adjustedByteCount < byteCount as returning null

tannergooding · 2021-06-17T22:57:55Z

@jkotas, @stephentoub This required a couple additional fixes since your sign-off to ensure it compiled on FreeBSD and so tests were passing everywhere. Would appreciate if you could take a look.

I believe it is otherwise good now and is just waiting on the last couple legs to complete

jkotas

LGTM

dotnet-issue-labeler bot added area-Interop-coreclr new-api-needs-documentation labels Jun 10, 2021

tannergooding commented Jun 10, 2021

View reviewed changes

src/coreclr/vm/nativememorynative.cpp Outdated Show resolved Hide resolved

tannergooding commented Jun 10, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.cs Outdated Show resolved Hide resolved

tannergooding commented Jun 10, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 10, 2021

View reviewed changes

src/coreclr/vm/corelib.cpp Outdated Show resolved Hide resolved

tannergooding force-pushed the native-memory branch 2 times, most recently from 1da201c to bd5e64e Compare June 10, 2021 17:55

tannergooding commented Jun 10, 2021

View reviewed changes

src/libraries/Common/src/Interop/Unix/System.Native/Interop.MemAlloc.cs Outdated Show resolved Hide resolved

tannergooding commented Jun 10, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.Unix.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 10, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.Unix.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 10, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.Unix.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 10, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.Windows.cs Outdated Show resolved Hide resolved

tannergooding commented Jun 10, 2021

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_memory.c Show resolved Hide resolved

jkotas reviewed Jun 11, 2021

View reviewed changes

src/libraries/Common/src/Interop/Windows/Ucrtbase/Interop.MemAlloc.cs Outdated Show resolved Hide resolved

tannergooding force-pushed the native-memory branch from af899a1 to 73af10a Compare June 11, 2021 20:39

tannergooding commented Jun 11, 2021

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_memory.c Show resolved Hide resolved

tannergooding commented Jun 11, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.Unix.cs Outdated Show resolved Hide resolved

tannergooding commented Jun 11, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.Unix.cs Show resolved Hide resolved

jkotas reviewed Jun 11, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.Unix.cs Show resolved Hide resolved

tannergooding force-pushed the native-memory branch from b76cc68 to f3ebd46 Compare June 11, 2021 21:32

jkotas reviewed Jun 11, 2021

View reviewed changes

...ies/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/NativeMemoryTests.cs Show resolved Hide resolved

tannergooding force-pushed the native-memory branch from f3ebd46 to d2ae1a1 Compare June 14, 2021 15:22

jkotas reviewed Jun 14, 2021

View reviewed changes

...ies/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/NativeMemoryTests.cs Outdated Show resolved Hide resolved

saucecontrol mentioned this pull request Jun 14, 2021

WIP. More Efficient MemoryAllocator SixLabors/ImageSharp#1660

Closed

4 tasks

jkotas reviewed Jun 15, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.Unix.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 15, 2021

View reviewed changes

src/libraries/Native/Unix/System.Native/config.h.in Outdated Show resolved Hide resolved

Fixing the Native\Unix\System.Native\CMakeLists.txt

924ea3c

stephentoub reviewed Jun 16, 2021

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_memory.c Show resolved Hide resolved

stephentoub reviewed Jun 16, 2021

View reviewed changes

src/libraries/Native/Unix/System.Native/pal_memory.c Show resolved Hide resolved

jkotas reviewed Jun 16, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeMemory.Unix.cs Outdated Show resolved Hide resolved

Update src/libraries/System.Private.CoreLib/src/System/Runtime/Intero…

1e07515

…pServices/NativeMemory.Unix.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>

stephentoub approved these changes Jun 16, 2021

View reviewed changes

This was referenced Jun 16, 2021

Update SystemNative_AlignedAlloc to properly check aligned_alloc availability for Apple targets #54296

Closed

Audit usages of Marshal.AllocHGlobal for ones that can be replaced with NativeMemory.Alloc #54297

Open

jkotas added area-System.Runtime.InteropServices and removed area-Interop-coreclr labels Jun 16, 2021

Don't call Buffer.Memmove in NativeMemory.AlignedRealloc if ptr is null

242ecca

jkotas reviewed Jun 16, 2021

View reviewed changes

tannergooding added 4 commits June 16, 2021 15:24

Updating NativeMemory.AlignedRealloc to correctly copy only the size …

b2601fe

…of the last allocation

Ensure check_symbol_exists(HAVE_ALIGNED_ALLOC) is under the non-apple…

2ee0409

… paths

Check for malloc_usable_size in malloc_np for FreeBSD and ensure test…

1266f4d

…s compile

Fix the ReallocSmallerToLargerTest test

05f6b92

Handle that posix_memalign differs from aligned_alloc for size == 0

805882b

tannergooding force-pushed the native-memory branch from 0e413a0 to 805882b Compare June 17, 2021 18:12

jkotas approved these changes Jun 17, 2021

View reviewed changes

tannergooding merged commit f721cf4 into dotnet:main Jun 18, 2021

iSazonov mentioned this pull request Jun 18, 2021

Review using Marshal.AllocHGlobal() PowerShell/PowerShell#15602

Closed

adamsitnik added this to the 6.0.0 milestone Jun 21, 2021

adamsitnik added the tenet-performance Performance related issue label Jun 21, 2021

mangod9 mentioned this pull request Jun 25, 2021

SingleFile asp.net apps throw an exception #54759

Closed

tannergooding mentioned this pull request Jul 15, 2021

What's new in .NET 6 Preview 7 dotnet/core#6444

Closed

AraHaan mentioned this pull request Jul 15, 2021

Removed CA1838 warning suppressions on p/invokes for StringBuilders. dotnet/winforms#4751

Closed

ghost locked as resolved and limited conversation to collaborators Jul 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement NativeMemory #54006

Implement NativeMemory #54006

tannergooding commented Jun 10, 2021

dotnet-issue-labeler bot commented Jun 10, 2021

tannergooding commented Jun 10, 2021

tannergooding commented Jun 15, 2021 •

edited

Loading

tannergooding commented Jun 16, 2021 •

edited

Loading

stephentoub left a comment

jkotas Jun 16, 2021

tannergooding Jun 16, 2021 •

edited

Loading

tannergooding Jun 16, 2021

tannergooding commented Jun 17, 2021

tannergooding commented Jun 17, 2021

jkotas left a comment

Implement NativeMemory #54006

Implement NativeMemory #54006

Conversation

tannergooding commented Jun 10, 2021

dotnet-issue-labeler bot commented Jun 10, 2021

tannergooding commented Jun 10, 2021

tannergooding commented Jun 15, 2021 • edited Loading

tannergooding commented Jun 16, 2021 • edited Loading

stephentoub left a comment

Choose a reason for hiding this comment

jkotas Jun 16, 2021

Choose a reason for hiding this comment

tannergooding Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

tannergooding Jun 16, 2021

Choose a reason for hiding this comment

tannergooding commented Jun 17, 2021

tannergooding commented Jun 17, 2021

jkotas left a comment

Choose a reason for hiding this comment

tannergooding commented Jun 15, 2021 •

edited

Loading

tannergooding commented Jun 16, 2021 •

edited

Loading

tannergooding Jun 16, 2021 •

edited

Loading