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

[API Proposal]: Expose Buffer.ZeroMemory(void*, nuint) #63547

Closed
Sergio0694 opened this issue Jan 9, 2022 · 11 comments · Fixed by #69500
Closed

[API Proposal]: Expose Buffer.ZeroMemory(void*, nuint) #63547

Sergio0694 opened this issue Jan 9, 2022 · 11 comments · Fixed by #69500
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory
Milestone

Comments

@Sergio0694
Copy link
Contributor

Sergio0694 commented Jan 9, 2022

Background and motivation

Buffer.ZeroMemory is currently internal (why?). This API just tunnels to SpanHelpers.ClearWithoutReferences, which is otherwise only accessible through new Span<byte>(void*, int).Clear(). This has two drawbacks: it's arguably clunkier than just doing Buffer.ZeroMemory(p, size), and (most importantly) caps the length of the buffer to int, which makes it immediately a no-go when dealing with larger buffers. This proposal is to make Buffer.ZeroMemory public to solve both issues.

API Proposal

namespace System;

public static class Buffer
{
    public static void ZeroMemory(void* buffer, nuint sizeInBytes);
}

API Usage

// When dealing with a short buffer

new Span<byte>(p, size).Clear(); // Before
Buffer.ZeroMemory(p, size); // After

// When dealing with a large buffer

// new Span<byte>(p, size).Clear(); // Unusable
Buffer.ZeroMemory(p, size); // Works just fine

Risks

None that I can see, really.

@Sergio0694 Sergio0694 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 9, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Memory untriaged New issue has not been triaged by the area owner labels Jan 9, 2022
@ghost
Copy link

ghost commented Jan 9, 2022

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

There are two APIs currently "missing" from the Buffer class:

  • Buffer.ZeroMemory is internal (why?). This API just tunnels to SpanHelpers.ClearWithoutReferences, which is otherwise only accessible through new Span<byte>(void*, int).Clear(). This has two drawbacks: it's arguably clunkier than just doing Buffer.ZeroMemory(p, size), and (most importantly) caps the length of the buffer to int, which makes it immediately a no-go when dealing with larger buffers. This proposal is to make Buffer.ZeroMemory public to solve both issues.
  • For parity, there should be a public overload of Buffer.MemoryCopy taking the size as a nuint.

API Proposal

namespace System;

public static class Buffer
{
    public static void MemoryCopy(void* source, void* destination, nuint destinationSizeInBytes, nuint sourceBytesToCopy);
    public static void ZeroMemory(void* buffer, nuint sizeInBytes);
}

API Usage

// When dealing with a short buffer

new Span<byte>(p, size).Clear(); // Before
Buffer.ZeroMemory(p, size); // After

// When dealing with a large buffer
// new Span<byte>(p, size).Clear(); // Unusable
Buffer.ZeroMemory(p, size); // Works just fine

Alternative Designs

No response

Risks

None that I can see, really.

Author: Sergio0694
Assignees: -
Labels:

api-suggestion, area-System.Memory, untriaged

Milestone: -

@adamsitnik
Copy link
Member

I like the idea of making Buffer.ZeroMemory public. 👍 @GrabYourPitchforks do you have any objections?

For parity, there should be a public overload of Buffer.MemoryCopy taking the size as a nuint

What would be the gain? The following code compiles just fine:

nuint byteCount = nuint.MaxValue;
void* ptr = IntPtr.Zero.ToPointer();

Buffer.MemoryCopy(ptr, ptr, byteCount, byteCount);

And I can't see any scenario where we could loose some data by casting nuint to ulong on 32bit system.

@bartonjs do we have any guidance for adding nint and nuint method overloads?

@adamsitnik adamsitnik removed the untriaged New issue has not been triaged by the area owner label Jan 10, 2022
@Sergio0694
Copy link
Contributor Author

"What would be the gain?"

The gain would be making it more consistent with other modern APIs (eg. all the ones in NativeMemory), technically more future proof and "correct" (ie. the point is not to have a 64bit value, but a native sized value), and possibly better to handle for the JIT, though not entirely sure on this last point (@SingleAccretion?). APIs taking int/uint/long/ulong to express sizes should really just remain a thing of the past, so I figured it'd be nice to add this one here to move in that direction. I'm sure @tannergooding will be able to argue for this point better than I possibly could though 😄

@bartonjs
Copy link
Member

bartonjs commented Jan 10, 2022

do we have any guidance for adding nint and nuint method overloads?

int is the preferred numeric type in .NET unless there's a reason to pick something else. That's about it as far as formal guidance, partially because n(u)int was added after we finished FDGv3.

If we already had 32 and 64 bit overloads then I could be convinced that adding an nuint overload would make sense, to avoid using a double-register on 32 bit systems... but we don't, it's just a 64-bit input (overloaded as signed and unsigned).

Since casting isn't required then I'd guess the review process would say something like "this is just a stylistic overload that feels like it costs more in the size increase to System.Private.Corelib than it has benefit to the ecosystem". But if (e.g.) Jan K wants to endorse it as good because of register allocations on 32-bit systems, then we'd nod and take his word on it.

@jkotas
Copy link
Member

jkotas commented Jan 10, 2022

But if (e.g.) Jan K wants to endorse it as good because of register allocations on 32-bit systems, then we'd nod and take his word on it.

The preferred way to copy and clear memory is Span.TryCopyTo and Span.Clear. I think the JIT is able to do a good job with registration allocation to make the Span overhead to disappear if you are starting with pointer and int size.

These overloads are working around the fact that Span length is int and not nuint. Do we believe that these are the only overloads in the BCL that need to be working around this limitation? How many other similar overloads we may add over time to work around this limitation?

@tannergooding
Copy link
Member

tannergooding commented Jan 11, 2022

These overloads are working around the fact that Span length is int and not nuint. Do we believe that these are the only overloads in the BCL that need to be working around this similar? How many other similar overloads we may add over time to work around this limitation?

Ideally not a lot, I think we end up with "at most" 1 "nuint" alternative to each Span API.

That being said, an alternative would be to just expose NativeSpan<T>, possibly in .NET 7 if we are actually getting ref fields, and most of this just becomes "if you need 'big data', use NativeSpan".
This would also have the benefit that Span<T> just "forwards" to NativeSpan internally (since its just a zero extension of span length, which is known positive).
-- You can also have the same "forwarding" in the case where we don't have NativeSpan; we just would need to decide where to centralize everything.

@tannergooding
Copy link
Member

Noting that if we don't expose NativeSpan<T> and ref fields are a thing, then I can think of at least 3 different libraries that are likely going to roll their own as soon as ref fields are available.

@GrabYourPitchforks
Copy link
Member

Agree with #63547 (comment) that there's not any real benefit to exposing nuint overloads of existing methods, which are typed as [u]long. Just leave those as legacy and introduce a new nuint API for the clear method.

@GrabYourPitchforks GrabYourPitchforks added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 13, 2022
@GrabYourPitchforks GrabYourPitchforks added this to the 7.0.0 milestone Apr 13, 2022
@Sergio0694 Sergio0694 changed the title [API Proposal]: Expose Buffer.ZeroMemory, add "modern" MemoryCopy overload [API Proposal]: Expose Buffer.ZeroMemory(void*, nuint) Apr 13, 2022
@Sergio0694
Copy link
Contributor Author

I've updated the API proposal as requested 👍

@terrajobst
Copy link
Member

terrajobst commented Apr 14, 2022

  • Makes sense but we believe it should live on NativeMemory instead.
namespace System.Runtime.InteropServices;

public static class NativeMemory
{
    public static void ZeroMemory(void* ptr, nuint byteCount);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 14, 2022
@Sergio0694
Copy link
Contributor Author

@tannergooding @GrabYourPitchforks I'm happy to take this if it's up for grabs 😄

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 18, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 20, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants