API proposal: SequenceReader.TryReadTo(out ReadOnlySpan<T> sequence, ReadOnlySpan<T> delimiter)

[davidfowl]

Today there's no overload of TryReadTo that returns (via out parameter) a ReadOnlySpan<T> and allows passing a ReadOnlySpan<T> delimeter.

public partial ref struct SequenceReader
{
    public bool TryReadTo(out ReadOnlySpan<T> span, ReadOnlySpan<T> delimiter, bool advancePastDelimiter = true);
}

@JeremyKuhne do you recall why we left this out?

[gfoidl]

To which type this should apply? I assume SequenceReader<T>.

out ReadOnlySpan<T> sequence

sequence reminds me of ReadOnlySequence<T>, so maybe span, to be in line with https://github.com/dotnet/corefx/blob/8ee61dd103ff55445f8baf3137bb768ed86370d9/src/System.Memory/src/System/Buffers/SequenceReader.Search.cs#L19

[davidfowl]

@gfoidl fixed!

[JeremyKuhne]

@JeremyKuhne do you recall why we left this out?

We were a little pressed for time, it might have fallen off due to additional complexity to optimize that case.

[terrajobst]

Video

• We have an existing API that returns the sequence.
• We don't want to offer an API that might allocate unexpectedly, especially on a type that is about avoiding allocations.

namespace System.Buffers
{
    public ref struct SequenceReader<T>
    {
        // Existing API
        // public bool TryReadTo(out ReadOnlySequence<T> sequence, T delimiter, bool advancePastDelimiter = true);

        public bool TryReadTo(out ReadOnlySpan<T> span, ReadOnlySpan<T> delimiter, bool advancePastDelimiter = true);
    }
}

[davidfowl]

Uhhhhh yes we do.

[terrajobst]

Uhhhhh yes we do.

Watch the video, I kind of predicted you'd say that :-)

[benaadams]

Every other TryRead api has an (out ReadOnlySpan<T> span, except multielement delimiter e.g. \r\n which only has a (out ReadOnlySequence<T> sequence, variant

namespace System.Buffers
{
    public ref struct SequenceReader<T>
    {
        // Existing api pairs: out ReadOnlySequence
        public bool TryReadTo(out ReadOnlySequence<T> sequence, T delimiter, bool advancePastDelimiter = true);
        public bool TryReadTo(out ReadOnlySequence<T> sequence, T delimiter, T delimiterEscape, bool advancePastDelimiter = true);
        public bool TryReadToAny(out ReadOnlySequence<T> sequence, ReadOnlySpan<T> delimiters, bool advancePastDelimiter = true);

        // Existing api pairs: out ReadOnlySpan
        public bool TryReadTo(out ReadOnlySpan<T> span, T delimiter, bool advancePastDelimiter = true);
        public bool TryReadTo(out ReadOnlySpan<T> span, T delimiter, T delimiterEscape, bool advancePastDelimiter = true);
        public bool TryReadToAny(out ReadOnlySpan<T> span, ReadOnlySpan<T> delimiters, bool advancePastDelimiter = true);

        // Missing pair: out ReadOnlySpan
        public bool TryReadTo(out ReadOnlySequence<T> sequence, ReadOnlySpan<T> delimiter, bool advancePastDelimiter = true)
    }
}

QED it is a missing api and the video didn't address that and

if (reader.TryReadTo(out sequence, delimiter, advancePastDelimiter: true))
{
    ReadOnlySpan<T> span = sequence.IsSingleSegment ? sequence.First.Span : sequence.ToArray();
}

Is way more inefficient than what the other methods do which is search the existing SequenceReader's internal span first, it its there return the sliced span, otherwise look at the ReadOnlySequence<T>; the common fast-path won't even touch the ROS.

[davidfowl]

I'm going to re-open this so we can re-discuss it 😄 . As @benaadams said, the other overloads do this, we should suggest an alternative pattern if we don't like this one but we need something.

[reflectronic]

I brought up how we rejected the ReadOnlySpan version of #30778 for this same reason reason in the chat during API review. If we go through with this API, we should look at that API again as well. I talked about my opinions on the Span returning APIs in that issue #30778 (comment)

[bartonjs]

Video

After much debate, approved as proposed:

public partial ref struct SequenceReader
{
    public bool TryReadTo(out ReadOnlySpan<T> span, ReadOnlySpan<T> delimiter, bool advancePastDelimiter = true);
}

[benaadams]

@bartonjs to follow up on api review, you can't mimic the behaviour with an extension method and the ReadOnlySequence<T> overload; because the SequenceReader has a ReadOnlySpan<T> internally so it has a much faster path when its contained in the current page as it already has the span and doesn't need to access the sequence; whereas an extension would always be starting on a slow path