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

Conversation

reflectronic
Copy link
Contributor

@reflectronic reflectronic commented Jun 21, 2021

Contributes to #54297

Probably best to review commit-by-commit, I tried not to backtrack too much.

Usages in tests will be addressed in a separate PR.


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.

@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@jeffhandley
Copy link
Member

@tannergooding This PR is assigned to you for follow-up/decision before the RC1 snap.

@danmoseley
Copy link
Member

@reflectronic you might prefer to push commits in groups, as each push restarts the build and test.

@reflectronic
Copy link
Contributor Author

reflectronic commented Jul 30, 2021

Yes; I am sorry, just got a little lazy, been fixing these last errors on GitHub.

@jeffhandley jeffhandley added this to the 7.0.0 milestone Aug 19, 2021
@ghost ghost closed this Sep 18, 2021
@ghost
Copy link

ghost commented Sep 18, 2021

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta community-contribution Indicates that the PR has been added by a community member
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

6 participants