Add MessagePackReader/Writer(Span<byte>) overload#2054
Closed
neuecc wants to merge 1 commit into
Closed
Conversation
AArnott
approved these changes
Nov 2, 2024
| else | ||
| { | ||
| // using only Span, can't create ReadOnlySequence so requires copy memory | ||
| this.sequence = new ReadOnlySequence<T>(this.CurrentSpan.ToArray()); |
Collaborator
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
MessagePackWriterandMessagePackReaderto support Span operations.Since the Writer is expected to auto-expand, I made it throw an
InvalidOperationExceptionwhen expansion is needed.While Try-pattern APIs might be better, this current approach isn't necessarily bad.
I also added a public
long WrittenCountproperty 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 addT[]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).