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

c#: Use span and memory apis for primative type parameters #1138

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jsturtevant
Copy link
Collaborator

@jsturtevant jsturtevant commented Jan 21, 2025

Fixes: #1080 and follow up to #1122.

Putting in draft for now since it has parts of #1137.

This generates both Span and Memory function signatures for and function that has a canonical list type.

Note that no tests have to change since the memory/span API' provide implicit operators that automatically convert Byte Arrays to Memory. But I added an additional check to the test.

Signed-off-by: James Sturtevant <[email protected]>
@pavelsavara
Copy link

Streams also support Span<>

https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.write?view=net-9.0#system-io-stream-write(system-readonlyspan((system-byte)))

Rule #1: For a synchronous API, use Span<T> instead of Memory<T> as a parameter if possible.
From
https://learn.microsoft.com/en-us/dotnet/standard/memory-and-spans/memory-t-usage-guidelines

Maybe for some things we could generate both ?

@jsturtevant
Copy link
Collaborator Author

jsturtevant commented Jan 22, 2025

Streams also support Span<>

https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.write?view=net-9.0#system-io-stream-write(system-readonlyspan((system-byte)))

This is for the synchronous api though? I was thinking since we don't really support synchronous API's and we have p3 async coming down the line we might now be able to use Span anyways. I am not sure here.

Rule #1: For a synchronous API, use Span instead of Memory as a parameter if possible.
From
https://learn.microsoft.com/en-us/dotnet/standard/memory-and-spans/memory-t-usage-guidelines

Maybe for some things we could generate both ?

The issue I ran into was when list is used in a variant. I was initially going to generate both but then this seemed like an option. Reading up on Memory it sounds like it makes small heap based allocations for every memory held so this might not be the right option for a default.

I will look to see if I can only generate this in the case of parameters and not for Variant based versions. I tried once but have a much better understanding of how it all comes together now.

@jsturtevant
Copy link
Collaborator Author

I've pushed a version that users Spans for any lists of primitive types and uses Fixed statements. PTAL

I think we will want/need to generate a second function that takes memory as well. The write use case is in the Stream API's WriteAsync method that takes Memory<T> and you cannot go from span to Memory?

@jsturtevant jsturtevant changed the title c#: Use memory api c#: Use span and memory apis for primative type parameters Jan 23, 2025
Signed-off-by: James Sturtevant <[email protected]>
Signed-off-by: James Sturtevant <[email protected]>
@pavelsavara
Copy link

This is for the synchronous api though? I was thinking since we don't really support synchronous API's and we have p3 async coming down the line we might now be able to use Span anyways. I am not sure here.

You are right, sorry. We can't have sync/blocking stream APIs until we have multi-threading.

(Unless we are happy to block on the stream operation, blocking also other async tasks in the ST thread-pool. And I have strong opinion against that.)

@jsturtevant
Copy link
Collaborator Author

I've added additional functions so we can support Span and memory for arrays of primitive types like u8, etc.

Signed-off-by: James Sturtevant <[email protected]>
@jsturtevant jsturtevant marked this pull request as ready for review January 24, 2025 00:44
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.

c#: Use Spans instead of e.g. byte[]
2 participants