Skip to content

Add MessagePackReader/Writer(Span<byte>) overload#2054

Closed
neuecc wants to merge 1 commit into
developfrom
span-reader-writer
Closed

Add MessagePackReader/Writer(Span<byte>) overload#2054
neuecc wants to merge 1 commit into
developfrom
span-reader-writer

Conversation

@neuecc

@neuecc neuecc commented Nov 1, 2024

Copy link
Copy Markdown
Member

While the addition of MessagePackPrimitives is proving challenging (ref: #1986),
what's really needed for practical use cases is the ability to Write/Read to/from Span.

For example, in my network framework MagicOnion, while we're reading and writing headers in MessagePack binary format, there are cases where we want to read/write directly with Span.

Therefore, I adapted MessagePackWriter and MessagePackReader to support Span operations.
Since the Writer is expected to auto-expand, I made it throw an InvalidOperationException when expansion is needed.
While Try-pattern APIs might be better, this current approach isn't necessarily bad.

I also added a public long WrittenCount property since we need to be able to get the amount written.

For the Reader, since some APIs in SequenceReader require a Sequence, I temporarily implemented it to create a Sequence using ToArray.
While this is better than throwing an exception, it's not the optimal solution.

Additionally, since ReadBytes requires generating a ReadOnlySequence, I added TryReadBytes that returns a raw Span.
This should be a required API for symmetry, as TryReadStringSpan already exists.

Making MessagePackReader compatible with ReadOnlySpan enables the addition of MessagePackSerializer.Deserialize<T>(ReadOnlySpan<byte>), though this API isn't included in this PR.
The inability to pass Span to Deserialize is a shortcoming in MessagePack for C#'s API that needs to be addressed.

Note that when both ReadOnlySpan and ReadOnlyMemory overloads exist, passing a byte[] causes overload resolution errors, so we need to either add T[] overloads or resolve it with OverloadResolutionPriority (though the latter is likely impractical).

More advanced solution proposal:
Remove ReadOnlySequence generation from BufferReader's public API to make everything Span-based. This would eliminate the need for Span copying and allow removing ReadOnlyMemory from overloads (though it might need to be kept for compatibility reasons).

@neuecc
neuecc requested a review from AArnott November 1, 2024 11:12
else
{
// using only Span, can't create ReadOnlySequence so requires copy memory
this.sequence = new ReadOnlySequence<T>(this.CurrentSpan.ToArray());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As an optimization opportunity going forward, every caller outside this class slices the result right away, so copying the whole thing here when the caller may only want a slice seems suboptimal. We could offer a method that returns a slice for the caller, and thereby copy only what we need.

@neuecc neuecc mentioned this pull request Nov 5, 2024
1 task
@neuecc neuecc closed this Dec 10, 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.

2 participants