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

Add support to ByteString for copying to/from Memory and Span #6026

Conversation

Arkatufus
Copy link
Contributor

Changes

Add CopyTo and CopyFrom Memory and Span support to ByteString

@to11mtm
Copy link
Member

to11mtm commented Jul 9, 2022

Should we consider going away from ArraySegment for internal storage and use Memory instead? On one hand it would be a bit of a yak shave, on the other hand it should greatly simplify the internal logic (since with Memory we can slice and not have to consider offset/length in other methods)

@Aaronontheweb
Copy link
Member

Should we consider going away from ArraySegment for internal storage and use Memory instead? On one hand it would be a bit of a yak shave, on the other hand it should greatly simplify the internal logic (since with Memory we can slice and not have to consider offset/length in other methods)

This is more what I was thinking too - moving away from ArraySegment and towards System.Memory primitives. I haven't reviewed this PR in any depth at all though.

@Arkatufus
Copy link
Contributor Author

I don't think we will get any improvement switching over to Memory, even microsoft ReadOnlySequence still uses ArraySegment underneath.

@Aaronontheweb
Copy link
Member

I don't think we will get any improvement switching over to Memory, even microsoft ReadOnlySequence still uses ArraySegment underneath.

being able to work with new native APIs in .NET 6 without copying is a big improvement: https://docs.microsoft.com/en-us/dotnet/api/system.net.sockets.socket.receiveasync?view=net-6.0#system-net-sockets-socket-receiveasync(system-memory((system-byte))-system-net-sockets-socketflags-system-threading-cancellationtoken)

@Arkatufus
Copy link
Contributor Author

Hmmm... true, as long as we ditch dotnetty

@to11mtm
Copy link
Member

to11mtm commented Jul 14, 2022

I don't think we will get any improvement switching over to Memory, even microsoft ReadOnlySequence still uses ArraySegment underneath.

being able to work with new native APIs in .NET 6 without copying is a big improvement: https://docs.microsoft.com/en-us/dotnet/api/system.net.sockets.socket.receiveasync?view=net-6.0#system-net-sockets-socket-receiveasync(system-memory((system-byte))-system-net-sockets-socketflags-system-threading-cancellationtoken)

Long term I think it's probably worth the payoff to just move to ReadOnlyMemory in place of ArraySegment.

Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

LGTM

@Aaronontheweb Aaronontheweb enabled auto-merge (squash) January 3, 2023 17:36
@Aaronontheweb Aaronontheweb added this to the 1.5.0 milestone Jan 3, 2023
@Aaronontheweb
Copy link
Member

This is small PR and doesn't do much to change the internals of ByteString - we should address the internals of this later, once we are in a better position to support System.Memory throughout the serialization and transport system.

@Aaronontheweb Aaronontheweb disabled auto-merge January 3, 2023 18:49
@Aaronontheweb Aaronontheweb merged commit 965e4c3 into akkadotnet:dev Jan 3, 2023
@Arkatufus Arkatufus deleted the Add_Memory_and_Span_support_to_ByteString branch February 27, 2023 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants