-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Api Proposal: Add reader for ReadOnlySequence<T> #27522
Comments
cc: @KrzysztofCwalina, @joshfree, @ahsonkhan, @davidfowl, @pakrym, @terrajobst, @danmosemsft, @stephentoub |
We need a reader for both ROS and Span. Why cannot this type have a ctor overload taking a Span? |
Taking Taking |
Position would work just fine with Span. The sequence property does not seem to be very useful. And there is no reason to use TryRead returning ROS when you read a single span. |
How do you create a
It is important in saving state of the Reader or resetting to the beginning.
Unless you take the reader as an argument in a helper method that never allocates or wants to pass ROS to some other method. I think have a two-state reader would be a problem- significant chunks of this struct would have either no meaning or limited value. The biggest value this struct brings is in handling the multi-segment complexity. It might be useful to aggregate the various span helpers ( |
The caller has the original ROS; they passed it to the ctor most likely. Why do they need the property to get it?
Is this a real and important scenario? I think there is value in having smaller number of these reader types. The value might be worth the tradeoff, i.e. limiting some scenarios where it's not clear we need them. I am not sure if this is doable (the right tradeoff), but I think we should think it's worth spending some more time thinking how we could ship a single reader type. It seems like you are giving up too easily :-) |
While I agree in principle, there is also value in not providing types that solve multiple things poorly.
I don't think I am. I know that handling Span will make this type more complicated and harder to use. I think having large swaths of the struct be non-usable in some cases is alone a non-starter. Take all these together and I have a hard time justifing spending the time:
What I could see doing is providing BufferReader that contains either a SequenceReader or a SpanReader and only exposes the always useful APIs. |
Have you measured it? As we talked, my mental model is that the methods simply read from the unread span (in both modes) and the only difference between the modes (from perf perspective) is when you hit the ned of the current span.
When we shipped the static methods, people (@jkotas and @stephentoub) felt is critical that we have a reader.
Is that objection different than point 3 above?
I am not sure I understand. Does your design support such context or it would have to be added to support spans. |
Yes- we can make the code much faster if we never have to go down the road of looking for additional space. Everything I measured (literally weeks of running perf tests) showed impact from the smallest of changes- even one condition would make a difference. I whipped together the start of SpanReader and got the following on the first thing I checked (these are going through the same 100,000 byte backing array):
Or, perhaps more interestingly, if I mark the method as
Hitting the no-more-data path is obviously really rare in this test. As such the difference in speed is almost entirely from the simple changes to the fast path. I'll put up a PR tomorrow that you can look at once I add a few more methods and tests, but here are the not-so-different implementation differences (of the fast path only): public static unsafe bool TryRead<T>(ref this BufferReader<byte> reader, out T value) where T : unmanaged
{
ReadOnlySpan<byte> span = reader.UnreadSpan;
if (span.Length < sizeof(T))
return TryReadSlow(ref reader, out value);
value = MemoryMarshal.Read<T>(span);
reader.Advance(sizeof(T));
return true;
}
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public void Advance(int count)
{
if (count < 0)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.length);
}
Consumed += count;
if (CurrentSpanIndex < CurrentSpan.Length - count)
{
CurrentSpanIndex += count;
}
else
{
// Current segment doesn't have enough space, scan forward through segments
AdvanceToNextSpan(count);
}
} public static unsafe bool TryRead<T>(ref this SpanReader<byte> reader, out T value) where T : unmanaged
{
ReadOnlySpan<byte> span = reader.UnreadSpan;
if (span.Length < sizeof(T))
{
value = default;
return false;
}
value = MemoryMarshal.Read<T>(span);
reader.Advance(sizeof(T));
return true;
}
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public void Advance(int count)
{
if (count < 0 || count > UnreadSpan.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.length);
}
Consumed += count;
UnreadSpan = UnreadSpan.Slice(count);
}
The current design doesn't need any extra context to save state. If we removed some of the members you'd need to hold onto extra data to save state. |
Changing the name to |
@JeremyKuhne, given dotnet/corefx#33288, can this issue be closed now or is there still some work left? If so, can we break out what's left into a separate issue? |
Rationale and usage
Reading data out of a
ReadOnlySequence<T>
(such as those returned viaSystem.IO.Pipelines.ReadResult
) is difficult. Writing performant, correct logic that handles data spanning multiple discontinuous segments is hard.SequenceReader<T>
wraps aReadOnlySequence<T>
and provides methods for reading binary and text data with a focus on performance and minimal or zero heap allocations.This proposal is based on code developed in CoreFXlabs
Take the sample code from the blog post introducing System.IO.Pipelines:
Dealing with slicing and
SequencePosition
is awkward. WithSequenceReader<byte>
the sample would be written like this:C# static local methods will allow making this particular example even cleaner as you'll be able to pull the reader (which is
ref
) into a local method when usingasync
.Here is another example of how the reader would be used to split a UTF-8 sequence into CRLF delimited lines, rejecting any loose or reversed line end characters.
To pause / recontinue reading the input data should be sliced. For example:
Key design considerations
SequenceReader<T>
is designed with the following considerations.a. The current
ReadOnlySpan<T>
needs to be cached to achieve this (which necessitatesSequenceReader<T>
beingref
)b. Reading data that is constrained to a single segment should be as fast as possible.
a. Using methods that don't return
ReadOnlySpan<T>
never heap allocate.b.
ReadOnlySpan<T>
a. Extension methods presume
SequenceReader<byte>
has UTF-8 data,SequenceReader<char>
has UTF-16 datab. UTF-16 functionality currently blocked on
Utf16Parser
shipping.unmanaged
andIEquatable<T>
a.
unmanaged
is necessary to do work on the stack (performance)b.
IEquatable<T>
is necessary to parseAPI surface area
API details
General
This API is based off of prototype code in CoreFXLab which has been tested against a prototype JSON reader.
advancePast
is provided as an optional parameter when searching as it can reduce subsequent calls toAdvance
. This has a measurable impact on performance.Methods that return
ReadOnlySequence<T>
never allocate.Methods that return
ReadOnlySpan<T>
will allocate when needed to aggregate.Any
methods are far faster when checking individualT
's up to 4 arguments.When at the end,
End
will be true and thePosition
will be at the end of the last segment.Peeking ahead
In order to be performant,
Peek
will return slices of existing data where possible.Parsing
The
Utf8Parser.TryParse
methods are open ended in what they'll read in.SequenceReader
will be capped at 128 items (bytes/chars). If we can't parse successfully with less it will be considered a failed parse. (See issues below)Futures
BufferWriter<T>
is still in early development, will work with existingIBufferWriter<T>
.String
convenience methods may be useful. (ReadLine
, etc.)Should look into providing adapters for reading
Stream
(need to think how one creates as UTF-16).We should consider supporting Augmented Backus-Naur rule sets for parsing as they figure into many specifications. Something like:
Issue summary
Allocating spans cross segment
Currently we don't provide a way to specify how we allocate when aggregating a span across a segment boundary. It is presumed that such allocating won't be frequent, and in user cases where it would be the
ReadOnlySequence
overloads allow custom aggregation. We could add additional overrloads where you provide a destination buffer, but that introduces additional complexity in needing to return more state (i.e. was there no match or did we run out of buffer space).To allow another way to control allocations in the fast path (Span returning methods) we'll allow passing in a custom delegate to allocate. By default we'll just allocate new arrays if this isn't specified.
System.Buffers.BuffersExtensions.CopyTo
allows copying to a span, but in many cases we'll only need to slice. The proposal above adds a new extension method to address this.Missing Utf16Parser
We didn't ship
Utf16Parser
when we addedUtf8Parser
.Utf8Parser
works fundamentally different than historicalTryParse
functionality as it reads until it has a valid data set and stops at further invalid data (with a number of caveats). HistoricalTryParse
functionality requires all passed in data be valid.Parsing must be bounded
Utf8Parser
is effectively unbounded for reading in data. Numbers can have any number of insignificant zeroes in them, for example. It also does not return rich error state when it can't parse. This means that we can't easily know when we have failed due to needing more data. Trying to parse all available data is also a potential DOS as we would need to aggregate data up toint.MaxValue
to make a call that we would still have to shrug off asReadOnlySequence<T>
is constrained bylong
, notint
.SequenceReader<T>
parsing methods deal with this by capping parsing at 128 elements (for all currently implemented numeric types). If we can't get a successful parse fromUtf8Parser
with less than 128 elements,SequenceReader<T>
will consider the parse attempt to be unsuccessful.SequenceReader can't be used in async methods
As it is a ref struct. Other features, such as C# static local methods will mitigate this restriction.
Can't stackalloc without unsafe
SequenceReader can't recieve "safe" stackalloc spans
Span<byte> span = stackalloc byte[128]
.Peek()
requires you to use unsafe to use stackalloc'ed spans. Peek would be addressed by dotnet/csharplang#1710, which is currently on track for 3.0 and is reflected above (the readonly modifier). Other potential methods that would take an output span buffer that modify struct state (i.e. that would advance the reader) would require some way to annotate that they will not capture the output span. Here is an example usage that requires unsafe currently:SequenceReader is large
SequenceReader
contains over 90 bytes of fields (with alignment and twoSequencePosition
s, aReadOnlySequence
, aReadOnlySpan
, etc.). Passing by reference is necessary to get the best performance. What little can be done to reduce the size (maybe 4 bytes) introduces significant complexity (borrowing bits for example). Cutting the fields down further introduces significant drops in performance (e.g. needing to reseek with every call).SpanReader
Spans can't be wrapped in
ReadOnlySequence
, which makesSequenceReader
unusable withReadOnlySpan
(Memory<T>
can be wrapped, however). While it is technically possible to makeSequenceReader
two-state and have it also take aReadOnlySpan
, it has the following drawbacks:SequenceReader
can't be used (such as theReadOnlySequence
returning methods).SequenceReader
is significantly more costly than a Span specific reader would be (4x as slow, over 2x as large)SequenceReader
would get larger to hold the Span and the context (another 16 bytes likely).Presumption is that if you're starting with a Span, you'll often not be doing many reads and/or will want any overhead to be as small as possible. Buffers being passed in native interop calls being one scenario where this would be the case. It is often reading an int or a struct and getting a type and/or length that you'll use to iterate through the rest of the buffer.
Proposal is that from a performance & usability standpoint we should have a matching
SpanReader
that includes all of the properties/methods that make sense to Span. We can also explore adding aBufferReader
that has the intersection of the functionality for those that want to be able to handle any incoming data and are willing to take the extra overhead of an adapter.The text was updated successfully, but these errors were encountered: