Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace usages of Marshal.AllocHGlobal with malloc in libraries #54468

Closed
wants to merge 63 commits into from
Closed
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
10f15f7
System.Private.CoreLib
reflectronic Jun 20, 2021
61340eb
System.Net.Security
reflectronic Jun 20, 2021
96d410b
System.Net.NameResolution
reflectronic Jun 20, 2021
f93fe74
System.Net.NetworkInformation
reflectronic Jun 20, 2021
6badbdd
System.Net.Quic
reflectronic Jun 20, 2021
d2b561f
System.Net.HttpListener
reflectronic Jun 20, 2021
38824c5
nint casts
reflectronic Jun 20, 2021
8ec610e
System.Text.Encoding.CodePages
reflectronic Jun 20, 2021
2db0e86
intptr casts
reflectronic Jun 20, 2021
658bf85
System.Speech
reflectronic Jun 20, 2021
0930248
System.ServiceProcess.ServiceController
reflectronic Jun 20, 2021
89ded43
NativeMemoryHelper in System.Speech
reflectronic Jun 20, 2021
340735a
NativeMemoryHelper System.Text.Encoding.CodePages
reflectronic Jun 20, 2021
7038fca
Crypto
reflectronic Jun 20, 2021
453f15e
System.Reflection.Metadata
reflectronic Jun 20, 2021
325b20d
System.Net.Http.WinHttpHandler
reflectronic Jun 20, 2021
36c48a4
System.Management
reflectronic Jun 20, 2021
09d0361
System.Diagnostics.EventLog
reflectronic Jun 20, 2021
190fb6b
System.Drawing.Common
reflectronic Jun 20, 2021
673925a
System.DirectoryServices.Protocols
reflectronic Jun 20, 2021
29e37a5
System.DirectoryServices.AccountManagement
reflectronic Jun 20, 2021
2510792
System.DirectoryServices
reflectronic Jun 20, 2021
935a77a
System.Data.Odbc
reflectronic Jun 20, 2021
68a9dc7
System.Diagnostics.PerformanceCounter
reflectronic Jun 20, 2021
443c362
more System.Drawing.Common
reflectronic Jun 20, 2021
5dd3cf6
more System.Private.CoreLib
reflectronic Jun 21, 2021
8783e8c
Fix build break
reflectronic Jun 21, 2021
3184c15
Fix additional embarrassing mistakes
reflectronic Jun 21, 2021
6a0746a
Fix mistake
reflectronic Jun 21, 2021
59cfa9b
fixing
reflectronic Jun 21, 2021
f8d77c2
fix project files
reflectronic Jun 21, 2021
8bc626d
powershell doesn't know how to preserve BOM
reflectronic Jun 21, 2021
d52bb61
fix project files
reflectronic Jun 23, 2021
aa86b70
does SRM csproj have BOM?
reflectronic Jun 23, 2021
e6b6e8e
more csproj fixes
reflectronic Jun 23, 2021
f0fb97a
one last time
reflectronic Jun 23, 2021
3423d98
Some changes
reflectronic Jun 23, 2021
b5dd0da
revert System.DirectoryServices.Protocols
reflectronic Jun 30, 2021
8a1f781
revert System.DirectoryServices.AccountManagement
reflectronic Jun 30, 2021
6fe4d7c
revert System.DirectoryServices
reflectronic Jun 30, 2021
c2275fc
fix corelib and other stuff
reflectronic Jun 30, 2021
068248c
revert System.Drawing.Common
reflectronic Jun 30, 2021
b875528
fix SafeNativeMemoryHandle
reflectronic Jun 30, 2021
f89bc04
Sysem.Text.Encoding.CodePages safenativememoryhandle
reflectronic Jun 30, 2021
2a961d3
fixxxx
reflectronic Jun 30, 2021
1b0f51a
do it again
reflectronic Jun 30, 2021
af272a8
Improve crypto
reflectronic Jun 30, 2021
a2042c6
EventLog and other stuff
reflectronic Jul 14, 2021
6e57302
misc other updates
reflectronic Jul 14, 2021
db1ee0c
Back out System.ServiceProcess
reflectronic Jul 14, 2021
e79d136
final fix perhaps
reflectronic Jul 14, 2021
b7c3292
Merge branch 'main' into replace-allochglobal
reflectronic Jul 14, 2021
7156b1e
source.dot.net lied to me
reflectronic Jul 14, 2021
3ea5947
Update MsQuicStream.cs
reflectronic Jul 14, 2021
fc6147a
fix
reflectronic Jul 14, 2021
1b3e561
Compile errors
reflectronic Jul 30, 2021
1f60dbf
Another fix
reflectronic Jul 30, 2021
852508b
Fix
reflectronic Jul 30, 2021
332e3aa
add unsafe
reflectronic Jul 30, 2021
de3481e
revert ldap interop
reflectronic Jul 30, 2021
8d3fbdd
temp change
reflectronic Jul 30, 2021
c31c674
improvement
reflectronic Jul 30, 2021
8a12cec
add unsafe
reflectronic Jul 30, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ internal static unsafe void Sysctl(Span<int> name, ref byte* value, ref int len)

private static unsafe void Sysctl(int* name, int name_len, ref byte* value, ref int len)
{
IntPtr bytesLength = (IntPtr)len;
IntPtr bytesLength = (nint)len;
int ret = -1;
bool autoSize = (value == null && len == 0);

Expand All @@ -45,7 +45,7 @@ private static unsafe void Sysctl(int* name, int name_len, ref byte* value, ref
{
throw new InvalidOperationException(SR.Format(SR.InvalidSysctl, *name, Marshal.GetLastWin32Error()));
}
value = (byte*)Marshal.AllocHGlobal((int)bytesLength);
value = (byte*)NativeMemory.Alloc((nuint)(nint)bytesLength);
}

ret = Sysctl(name, name_len, value, &bytesLength);
Expand All @@ -54,27 +54,27 @@ private static unsafe void Sysctl(int* name, int name_len, ref byte* value, ref
// Do not use ReAllocHGlobal() here: we don't care about
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: ReAllocHGlobal

// previous contents, and proper checking of value returned
// will make code more complex.
Marshal.FreeHGlobal((IntPtr)value);
NativeMemory.Free(value);
if ((int)bytesLength == int.MaxValue)
{
throw new OutOfMemoryException();
}
if ((int)bytesLength >= int.MaxValue / 2)
{
bytesLength = (IntPtr)int.MaxValue;
bytesLength = (nint)int.MaxValue;
}
else
{
bytesLength = (IntPtr)((int)bytesLength * 2);
bytesLength = (nint)((int)bytesLength * 2);
}
value = (byte*)Marshal.AllocHGlobal(bytesLength);
value = (byte*)NativeMemory.Alloc((nuint)(nint)bytesLength);
ret = Sysctl(name, name_len, value, &bytesLength);
}
if (ret != 0)
{
if (autoSize)
{
Marshal.FreeHGlobal((IntPtr)value);
NativeMemory.Alloc((nuint)value);
}
throw new InvalidOperationException(SR.Format(SR.InvalidSysctl, *name, Marshal.GetLastWin32Error()));
}
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/Common/src/Interop/Interop.Ldap.cs
Original file line number Diff line number Diff line change
Expand Up @@ -172,12 +172,12 @@ internal sealed class LdapMod
{
if (attribute != IntPtr.Zero)
{
Marshal.FreeHGlobal(attribute);
NativeMemoryHelper.Free(attribute);
}

if (values != IntPtr.Zero)
{
Marshal.FreeHGlobal(values);
NativeMemoryHelper.Free(values);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public static int ber_printf_int(SafeBerHandle berElement, string format, int va
[DllImport(Libraries.OpenLdap, EntryPoint = "ber_put_boolean", CharSet = CharSet.Ansi)]
public static extern int ber_put_boolean(SafeBerHandle berElement, int value, nuint tag);

public static int ber_printf_bytearray(SafeBerHandle berElement, string format, HGlobalMemHandle value, nuint length, nuint tag)
public static int ber_printf_bytearray(SafeBerHandle berElement, string format, NativeMemoryHandle value, nuint length, nuint tag)
{
if (format == "o")
{
Expand All @@ -105,13 +105,13 @@ public static int ber_printf_bytearray(SafeBerHandle berElement, string format,
}

[DllImport(Libraries.OpenLdap, EntryPoint = "ber_put_ostring", CharSet = CharSet.Ansi)]
private static extern int ber_put_ostring(SafeBerHandle berElement, HGlobalMemHandle value, nuint length, nuint tag);
private static extern int ber_put_ostring(SafeBerHandle berElement, NativeMemoryHandle value, nuint length, nuint tag);

[DllImport(Libraries.OpenLdap, EntryPoint = "ber_put_string", CharSet = CharSet.Ansi)]
private static extern int ber_put_string(SafeBerHandle berElement, HGlobalMemHandle value, nuint tag);
private static extern int ber_put_string(SafeBerHandle berElement, NativeMemoryHandle value, nuint tag);

[DllImport(Libraries.OpenLdap, EntryPoint = "ber_put_bitstring", CharSet = CharSet.Ansi)]
private static extern int ber_put_bitstring(SafeBerHandle berElement, HGlobalMemHandle value, nuint length, nuint tag);
private static extern int ber_put_bitstring(SafeBerHandle berElement, NativeMemoryHandle value, nuint length, nuint tag);

[DllImport(Libraries.OpenLdap, EntryPoint = "ber_flatten", CharSet = CharSet.Ansi)]
public static extern int ber_flatten(SafeBerHandle berElement, ref IntPtr value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ private static unsafe void AllocNullTerminatedArray(string[] arr, ref byte** arr

// Allocate the unmanaged array to hold each string pointer.
// It needs to have an extra element to null terminate the array.
arrPtr = (byte**)Marshal.AllocHGlobal(sizeof(IntPtr) * arrLength);
arrPtr = (byte**)NativeMemory.Alloc((uint)(sizeof(IntPtr) * arrLength));
Debug.Assert(arrPtr != null);

// Zero the memory so that if any of the individual string allocations fails,
Expand All @@ -70,10 +70,10 @@ private static unsafe void AllocNullTerminatedArray(string[] arr, ref byte** arr
{
byte[] byteArr = Encoding.UTF8.GetBytes(arr[i]);

arrPtr[i] = (byte*)Marshal.AllocHGlobal(byteArr.Length + 1); //+1 for null termination
arrPtr[i] = (byte*)NativeMemory.Alloc((uint)(byteArr.Length + 1u)); //+1 for null termination
Debug.Assert(arrPtr[i] != null);

Marshal.Copy(byteArr, 0, (IntPtr)arrPtr[i], byteArr.Length); // copy over the data from the managed byte array
Marshal.Copy(byteArr, 0, (nint)arrPtr[i], byteArr.Length); // copy over the data from the managed byte array
arrPtr[i][byteArr.Length] = (byte)'\0'; // null terminate
}
}
Expand All @@ -87,13 +87,13 @@ private static unsafe void FreeArray(byte** arr, int length)
{
if (arr[i] != null)
{
Marshal.FreeHGlobal((IntPtr)arr[i]);
NativeMemory.Free(arr[i]);
arr[i] = null;
}
}

// And then the array itself
Marshal.FreeHGlobal((IntPtr)arr);
NativeMemory.Free(arr);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ internal static partial class Advapi32
internal static extern bool ConvertStringSecurityDescriptorToSecurityDescriptor(
string StringSecurityDescriptor,
int StringSDRevision,
out SafeLocalAllocHandle pSecurityDescriptor,
out SafeNativeMemoryHandle pSecurityDescriptor,
IntPtr SecurityDescriptorSize);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,15 @@ internal static partial class Advapi32
internal static extern bool GetTokenInformation(
SafeAccessTokenHandle TokenHandle,
uint TokenInformationClass,
SafeLocalAllocHandle TokenInformation,
SafeNativeMemoryHandle TokenInformation,
uint TokenInformationLength,
out uint ReturnLength);

[DllImport(Interop.Libraries.Advapi32, SetLastError = true)]
internal static extern bool GetTokenInformation(
IntPtr TokenHandle,
uint TokenInformationClass,
SafeLocalAllocHandle TokenInformation,
SafeNativeMemoryHandle TokenInformation,
uint TokenInformationLength,
out uint ReturnLength);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,11 @@ internal static CRYPT_OID_INFO FindOidInfo(CryptOidInfoKeyType keyType, string k
{
if (keyType == CryptOidInfoKeyType.CRYPT_OID_INFO_OID_KEY)
{
rawKey = Marshal.StringToCoTaskMemAnsi(key);
rawKey = NativeMemoryHelper.AllocStringAnsi(key);
}
else if (keyType == CryptOidInfoKeyType.CRYPT_OID_INFO_NAME_KEY)
{
rawKey = Marshal.StringToCoTaskMemUni(key);
rawKey = NativeMemoryHelper.AllocStringUnicode(key);
}
else
{
Expand Down Expand Up @@ -105,9 +105,9 @@ internal static CRYPT_OID_INFO FindOidInfo(CryptOidInfoKeyType keyType, string k
}
finally
{
if (rawKey != IntPtr.Zero)
if ((nint)rawKey != 0)
{
Marshal.FreeCoTaskMem(rawKey);
NativeMemoryHelper.Free(rawKey);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -515,17 +515,17 @@ public override int Size
get { return _size; }
}

public static SafeLocalFreeChannelBinding LocalAlloc(int cb)
public static unsafe SafeLocalFreeChannelBinding LocalAlloc(int cb)
{
SafeLocalFreeChannelBinding result = new SafeLocalFreeChannelBinding();
result.SetHandle(Marshal.AllocHGlobal(cb));
result.SetHandle(NativeMemory.Alloc((uint)cb));
result._size = cb;
return result;
}

protected override bool ReleaseHandle()
protected override unsafe bool ReleaseHandle()
{
Marshal.FreeHGlobal(handle);
NativeMemory.Free((void*)(nint)handle);
return true;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

namespace Microsoft.Win32.SafeHandles
{
internal sealed class SafeLocalAllocHandle : SafeBuffer
internal sealed class SafeLocalAllocHandle
: SafeBuffer
{
public SafeLocalAllocHandle() : base(true) { }

Expand All @@ -15,13 +16,13 @@ public SafeLocalAllocHandle() : base(true) { }
internal static SafeLocalAllocHandle LocalAlloc(int cb)
{
var h = new SafeLocalAllocHandle();
h.SetHandle(Marshal.AllocHGlobal(cb));
h.SetHandle(NativeMemoryHelper.Alloc(cb));
h.Initialize((ulong)cb);
return h;
}

// 0 is an Invalid Handle
internal SafeLocalAllocHandle(IntPtr handle) : base(true)
private SafeLocalAllocHandle(IntPtr handle) : base(true)
{
SetHandle(handle);
}
Expand All @@ -36,7 +37,7 @@ internal static SafeLocalAllocHandle InvalidHandle

protected override bool ReleaseHandle()
{
Marshal.FreeHGlobal(handle);
NativeMemoryHelper.Free(handle);
return true;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ internal sealed class SafeUnicodeStringHandle : SafeHandle
public SafeUnicodeStringHandle(string s)
: base(IntPtr.Zero, ownsHandle: true)
{
handle = Marshal.StringToHGlobalUni(s);
handle = NativeMemoryHelper.AllocStringUnicode(s);
}

public unsafe SafeUnicodeStringHandle(ReadOnlySpan<char> s)
Expand All @@ -32,7 +32,7 @@ public unsafe SafeUnicodeStringHandle(ReadOnlySpan<char> s)
{
int cch = checked(s.Length + 1);
int cb = checked(cch * sizeof(char));
handle = Marshal.AllocHGlobal(cb);
handle = NativeMemoryHelper.Alloc(cb);

Span<char> dest = new Span<char>(handle.ToPointer(), cch);
s.CopyTo(dest);
Expand All @@ -44,13 +44,13 @@ public sealed override bool IsInvalid
{
get
{
return handle == IntPtr.Zero;
return (nint)handle == 0;
}
}

protected sealed override bool ReleaseHandle()
{
Marshal.FreeHGlobal(handle);
NativeMemoryHelper.Free(handle);
return true;
}
}
Expand Down
115 changes: 115 additions & 0 deletions src/libraries/Common/src/System/NativeMemoryHelper.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

#nullable enable

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.InteropServices;
using System.Text;

namespace System
{
internal static unsafe class NativeMemoryHelper
Copy link
Member

@jkotas jkotas Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be called NativeMemory and only included for pre-.NET 6 targets. We do not want to have the wrapper on .NET 6+.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can work for Alloc and Free, but it also has some replacements for Marshal.StringToHGlobalUni that would have to stay on a type like this. Is that okay?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid touching StringToHGlobalUni in this PR. If it is useful to have NativeMemory variant of StringToHGlobalUni and similar APIs, we should introduce a proper public API for it first. Duplicating NativeMemory variant of StringToHGlobalUni via private helper is not an improvement over using StringToHGlobalUni directly.

Copy link
Member

@tannergooding tannergooding Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be worth profiling more and its probably worth opening a proposal.

On my box, a benchmark which does simply the following, over a 260 character string:

var ptr = Marshal.StringToHGlobalUni(s);
Marshal.FreeHGlobal(ptr);

Compared to an equivalent API using NativeMarshal.Alloc/Free under the covers:

BenchmarkDotNet=v0.13.0, OS=Windows 10.0.19043.1052 (21H1/May2021Update)
AMD Ryzen 9 5950X, 1 CPU, 32 logical and 16 physical cores
.NET SDK=6.0.100-preview.5.21302.13
  [Host]     : .NET 6.0.0 (6.0.21.30105), X64 RyuJIT
  DefaultJob : .NET 6.0.0 (6.0.21.30105), X64 RyuJIT

|  Method |     Mean |    Error |   StdDev |
|-------- |---------:|---------:|---------:|
|   Alloc | 41.92 ns | 0.051 ns | 0.045 ns |
| HGlobal | 47.37 ns | 0.071 ns | 0.066 ns |

Of course, in comparison to the GC transition and any work the call itself does, this is probably not that meaningful but ~10% overhead can add up.

Copy link
Member

@tannergooding tannergooding Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • To clarify, I don't think anything needs to be done in this PR, just calling out there is a measurable difference here and so it might be worth opening a proposal because of that. In more extreme cases, many Alloc calls can be up to 20x faster than many LocalAlloc calls.

{
public static IntPtr Alloc(int byteCount)
{
#if NET6_0_OR_GREATER
return (nint)NativeMemory.Alloc((uint)byteCount);
#else
return Marshal.AllocHGlobal(byteCount);
#endif
}

public static IntPtr Realloc(IntPtr pointer, int byteCount)
{
#if NET6_0_OR_GREATER
return (nint)NativeMemory.Realloc((void*)(nint)pointer, (uint)byteCount);
#else
return Marshal.ReAllocHGlobal(pointer, (nint)byteCount);
#endif
}

public static IntPtr AllocStringUnicode(string? s)
{
if (s is null)
{
return IntPtr.Zero;
}

var byteCount = (s.Length + 1) * 2;

// Overflow checking
if (byteCount < s.Length)
{
ThrowArgumentOutOfRangeException(nameof(s));
}

#if NET6_0_OR_GREATER
char* memory = (char*)NativeMemory.Alloc((uint)byteCount);
s.CopyTo(new Span<char>(memory, s.Length));
#else
char* memory = (char*)(nint)Marshal.AllocHGlobal(byteCount);
// Avoid pulling in System.Memory for netstandard2.0 targets.
fixed (char* str = s)
{
Buffer.MemoryCopy(str, memory, byteCount, s.Length * 2);
}
#endif

memory[s.Length] = '\0';
return (nint)memory;
}

public static IntPtr AllocStringAnsi(string? s)
{
if (s is null)
{
return IntPtr.Zero;
}

long longByteCount = (s.Length + 1) * (long)Marshal.SystemMaxDBCSCharSize;
int byteCount = (int)longByteCount;

// Overflow checking
if (byteCount != longByteCount)
{
ThrowArgumentOutOfRangeException(nameof(s));
}

#if NET6_0_OR_GREATER
byte* memory = (byte*)NativeMemory.Alloc((uint)byteCount);
#else
byte* memory = (byte*)(nint)Marshal.AllocHGlobal(byteCount);
#endif

fixed (char* str = s)
{
int convertedBytes = Encoding.UTF8.GetBytes(str, s.Length, memory, byteCount);
memory[convertedBytes] = 0;
}

return (nint)memory;
}


public static void Free(IntPtr pointer)
{
#if NET6_0_OR_GREATER
NativeMemory.Free((void*)(nint)pointer);
#else
Marshal.FreeHGlobal(pointer);
#endif
}


#if NET6_0_OR_GREATER
[StackTraceHidden]
#endif
[DoesNotReturn]
private static void ThrowArgumentOutOfRangeException(string argument)
{
throw new ArgumentOutOfRangeException(argument);
}
}
}
Loading