Add ToReadOnlySequence() to ByteString #7491
Conversation
src/core/Akka/Util/ByteString.cs
Outdated
| if (bArr.Length == 2) | ||
| { | ||
| item = bArr[1]; | ||
| last = new ByteStringReadOnlySequenceSegment(item, bs.Count-item.Count); | ||
| first.Next = last; | ||
| } |
There was a problem hiding this comment.
TBH this might be premature optimization....
src/core/Akka/Util/ByteString.cs
Outdated
| var prior = first; | ||
| for (int i = 1; i < bArr.Length; i++) | ||
| { | ||
| item = bArr[i]; | ||
| var curr = new ByteStringReadOnlySequenceSegment(item, prior.RunningIndex+prior.Memory.Length); | ||
| prior.Next = curr; | ||
| prior = curr; | ||
| } | ||
| last = prior; | ||
| } | ||
| return new ReadOnlySequence<byte>(first,0,last,last.Memory.Length); |
There was a problem hiding this comment.
IDK if I like this less or more than just 'going backwards on the array' and just building the Segments from last to first.
Happy for feedback (I suppose main thing being the pattern here vs setting next explicitly in ctor could impact JIT optimizations... but probably overthinking things a bit.)
src/core/Akka/Util/ByteString.cs
Outdated
| [DebuggerDisplay("(Count = {_count}, Buffers = {_buffers})")] | ||
| public sealed class ByteString : IEquatable<ByteString>, IEnumerable<byte> | ||
| { | ||
| public class ByteStringReadOnlySequenceSegment : ReadOnlySequenceSegment<byte> |
There was a problem hiding this comment.
Not sure where to put in class (or maybe even move to partial/etc?)
It's a nested class for the sake of being able to directly access _buffers in the create. OTOH if we 'flipped' the construction (see other comment) we wouldn't need to nest it and could just build in .ToReadOnlySequence().
... Either way this should be sealed though
|
Needs API review |
|
All observed failures in tests appear to be races since they wouldn't be using these new APIs. |
There was a problem hiding this comment.
We are probably going to restructure ByteString to implement System.Memory directly, so anything that's more complicated than
public ReadOnlySequence<byte> ToReadOnlySequence()
{
if (_count == 0)
{
return ReadOnlySequence<byte>.Empty;
}
else if (_buffers.Length == 1)
{
//Happy path, we can just pass ArraySegment here and avoid alloc.
return new ReadOnlySequence<byte>(_buffers[0]);
}
else
{
return ByteStringReadOnlySequenceSegment.CreateSequence(this);
}
}needs to be deleted.
Did my best to simplify the creation path; If there's specific things to handle please LMK. |
|
Thanks @to11mtm |
2 participants
Changes
Adds
public ReadOnlySequence<byte> ToReadOnlySequence()toByteString.Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):
Latest
devBenchmarksN/A
This PR's Benchmarks
N/A
Notes:
ReadOnlySequenceis a bit of a tradeoff; On one hand, it is a larger struct (two objects and two ints) and does some fun typechecks against said objects which makes it not as fast asReadOnlySpan.On the other hand, for non-compacted
ByteString, rather than requiring a full copy+alloc of new array, it's essentially a linked-list of the segments (each segment being alongof the total length of previous segments, aMemory<T>and the nextReadOnlySequenceSegment).(For already 'compacted'
ByteStrings, no allocation is required sinceReadOnlySequencehas logic to to avoid allocation for a singleReadOnlyMemorybeing passed in.)The other advantage however, is that
ReadOnlySequenceis *notaref struct`.This makes it easier to do certain user code (i.e. not having to worry about
ref structrules as much, e.x. including in a class being passed as a message.As far as ecosystem support, I know MessagePack-CSharp supports
ReadOnlySequence<byte>for input, System.Text.Json should eventually include it based on dotnet/runtime#95187 / dotnet/runtime#67337, and Protobuf-Net appears to have support for it as well. Probably others too but those are the first that came to mind for serialization.