Skip to content

Conversation

@jlaanstra
Copy link
Collaborator

@jlaanstra jlaanstra commented Mar 12, 2024

Contributes to #824

WindowsRuntimeBuffer is now internally represented by a Memory<byte> instead of a byte[]. Also simplified a few codepaths to use the CopyTo functionality from Span.

AsMemory() follows the approach taken by ToArray() in that the capacity of the IBuffer is ignored and the Memory represents the length of the buffer.

@jlaanstra jlaanstra force-pushed the user/jlaans/memories branch from b41c126 to c9ee687 Compare March 12, 2024 01:57
@jlaanstra jlaanstra requested a review from manodasanW March 12, 2024 16:21
@jlaanstra jlaanstra force-pushed the user/jlaans/memories branch from e6e13e1 to 4d0d9dc Compare March 14, 2024 22:55
Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

I don't think we should expose these APIs as is, because they are effectively unsafe, but hide that. Having a custom memory manager wrapping an IBuffer is equivalent to having a memory manager with a finalizer (because IBuffer will have a finalizer), which is explicitly not recommended specifically because it's unsafe.

We should definitely expose a way to convert between IBuffer and Memory<T> types, but I'm wondering whether it wouldn't be better to do so via an API that makes the fact they're unsafe explicit, and with proper documentation, so developers can know what to do. For instance, something like this:

public static class WindowsRuntimeMarshal
{
    public static Memory<byte> AsMemoryUnsafe(IBuffer buffer);
}

Just for context, pasting it here again, this is the scenario I'm worried about:

IBuffer buffer = GetSomeBuffer();
Span<byte> span = buffer.AsMemory().Span;
// <-- GC runs here
DoStuff(span); // Boom 💥

Also cc. @tannergooding, @GrabYourPitchforks, thoughts? 🙂

@jlaanstra
Copy link
Collaborator Author

I don't think we should expose these APIs as is, because they are effectively unsafe, but hide that. Having a custom memory manager wrapping an IBuffer is equivalent to having a memory manager with a finalizer (because IBuffer will have a finalizer), which is explicitly not recommended specifically because it's unsafe.

We should definitely expose a way to convert between IBuffer and Memory<T> types, but I'm wondering whether it wouldn't be better to do so via an API that makes the fact they're unsafe explicit, and with proper documentation, so developers can know what to do. For instance, something like this:

public static class WindowsRuntimeMarshal
{
    public static Memory<byte> AsMemoryUnsafe(IBuffer buffer);
}

Just for context, pasting it here again, this is the scenario I'm worried about:

IBuffer buffer = GetSomeBuffer();
Span<byte> span = buffer.AsMemory().Span;
// <-- GC runs here
DoStuff(span); // Boom 💥

Also cc. @tannergooding, @GrabYourPitchforks, thoughts? 🙂

Doesn't this apply equally to string?

string buffer = GetSomeString();
Span<char> span = buffer.AsMemory().Span;
// <-- GC runs here
DoStuff(span); // Boom 💥

@Sergio0694
Copy link
Member

"Doesn't that equally apply to string?"

No, because a string is a managed object and the span will keep it alive (as it's an interior pointer to it).

@jlaanstra
Copy link
Collaborator Author

jlaanstra commented Mar 26, 2024

Superseded by #1556

@jlaanstra jlaanstra closed this Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants