Shouldn't string reader return ReadOnlySpan<char> when reading line in StringReader

[Bartsz]

I am considering the addition of a ReadLineAsSpan method to the StringReader class. The current ReadLine method returns a string, likely for backward compatibility reasons. However, this approach seems to undermine the benefits of using spans within the StringReader class.

When reading a string line by line, the current implementation reallocates the entire string in chunks, which could be avoided by returning a ReadOnlySpan instead.

With minimal modifications, the existing method public override string? ReadLine() would become public ReadOnlySpan<char> ReadLineAsSpan() and new ReadLine() would be created

public ReadOnlySpan<char> ReadLineAsSpan()
{
    // at line 190
    
    string result = s.Substring(pos, foundLineLength);
    //replaced by 
    ReadOnlySpan<char> result = span.Slice(pos, foundLineLength);
    //plus additional modification necessary for it to work 
}

And previous public override string? ReadLine() becomes

public override string? ReadLine() 
{
    ReadOnlySpan<char> lineSpan= ReadLineAsSpan();
    return lineSpan.IsEmpty ?  null : lineSpan.ToString();
}

This approach maintains backward compatibility while leveraging the performance benefits of spans.

Does this proposal make any sense? Am I missing something?

[elgonzo]

There is a little problem here: How would ReadLineAsSpan() be able to signal end of the string being reached vs. reading an empty line? This is not correct: return lineSpan.IsEmpty ? null : lineSpan.ToString(); as it would treat an empty line as end of string.

As a span type (ref struct) can't be wrapped in Nullable<T> nor be used in a value tuple, you would either need to introduce a boolean out parameter (or something similar) or use ReadOnlyMemory<char>? instead of ReadOnlySpan<char> to realize this. As i am however are not part of the .NET team, i have no particular awareness of the API design rules that would need to be adhered to.

[Bartsz]

Yeas I understand the issue with nullable, and I thought exactly the same thing and had the same concern about design rules.

So I checked the first few results on Bing 😉 to see how people use ReadLine , and it typically falls into one of the following patterns:

• while true - same as in official documentation

while(true)
{
    aLine = strReader.ReadLine();
    if(aLine != null)
    {
        // ...
    }
    // ...
}

• inline assignment - very common

 while ((line = reader.ReadLine()) != null)
 {
    // ...
 }

• Peek

 while (reader.Peek() != -1)
 {
    var line = reader.ReadLine()
    // ...
 }

To be honest, I don’t like any of these patterns (of course, this is just a style preference):

• While true is error-prone and doesn’t lend itself to code readability.
• Inline assignment is ugly and not necessarily obvious to the reader.
• Peek requires an understanding of the Peek() method.

I suggest providing a better API by creating a method (I’m not sure if IsEndOfString is a good name, maybe ReachedEndOfString):

public bool IsEndOfString()
{
    if (_s == null)
    {
        ThrowObjectDisposedException_ReaderClosed();
    }

    return _pos >= _s.Length;
}

You can then use ReadLineAsSpan() together with IsEndOfString:

 while (!reader.IsEndOfString())
 {
    var line = reader.ReadLineAsSpan()
    // ...
 }

When the end of the string is reached, it would work in the same way as ReadLine. ReadLine will return null indefinitely, just like ReadLineAsSpan would indefinitely return an empty span.

Here are the proposed minimal changes:

        public override string? ReadLine()
        {
            if (_s == null)
            {
                ThrowObjectDisposedException_ReaderClosed();
            }

            if ((uint)_pos >= (uint)s.Length)
            {
                return null;
            }

            return ReadLineAsSpan().ToString();
        }

        public bool IsEndOfString()
        {
            return _pos >= _s.Length;
        }

        public ReadOnlySpan<char> ReadLineAsSpan()
        {
            string? s = _s;
            if (s == null)
            {
                ThrowObjectDisposedException_ReaderClosed();
            }

            int pos = _pos;
            if ((uint)pos >= (uint)s.Length)
            {
                return ReadOnlySpan<char>.Empty; //CHANGED
            }

            ReadOnlySpan<char> remaining = s.AsSpan(pos);
            int foundLineLength = remaining.IndexOfAny('\r', '\n');
            if (foundLineLength >= 0)
            {
                ReadOnlySpan<char> result = remaining.Slice(0, foundLineLength);//CHANGED

                char ch = remaining[foundLineLength];
                pos += foundLineLength + 1;
                if (ch == '\r')
                {
                    if ((uint)pos < (uint)s.Length && s[pos] == '\n')
                    {
                        pos++;
                    }
                }
                _pos = pos;

                return result;
            }
            else
            {
                //string result = s.Substring(pos); REMOVED
                _pos = s.Length;
                return remaining;//CHANGED
            }
        }

[Bartsz]

I've run a benchmark for code above and I got results as below. ReadLineAsSpan would result in almost no allocation and 30% speed improvement.

  [Benchmark]
  public void ReaderReadLine()
  {
      var reader = new StringReader(content);

      while (true)
      {
          var line = reader.ReadLine();
          if (line == null)
          {
              break;
          }
      }
  }

  [Benchmark]
  public void ReaderReadLineAsSpan()
  {
      var reader = new StringReaderNew(content);

      while (!reader.IsEndOfString())
      {
          var line = reader.ReadLineAsSpan();
      }
  }

BenchmarkDotNet v0.14.0, Windows 11 (10.0.22631.4169/23H2/2023Update/SunValley3)
12th Gen Intel Core i7-1280P, 1 CPU, 20 logical and 14 physical cores
.NET SDK 8.0.402
  [Host]     : .NET 8.0.8 (8.0.824.36612), X64 RyuJIT AVX2
  Job-YXRZRJ : .NET 8.0.8 (8.0.824.36612), X64 RyuJIT AVX2

IterationCount=50  LaunchCount=10  RunStrategy=ColdStart

| Method               | Mean     | Error    | StdDev   | Median    | Allocated |
|--------------------- |---------:|---------:|---------:|----------:|----------:|
| ReaderReadLine       | 508.3 us | 59.14 us | 399.5 us | 442.50 us | 3120440 B |
| ReaderReadLineAsSpan | 336.2 us | 35.34 us | 238.7 us | 296.05 us |     432 B |

[MihaZupan]

FWIW if all you want to do is loop over the lines, you can also use

foreach (var line in span.EnumerateLines())

Which may also save you from allocating the string in the first place.

(note that EnumerateLines looks for more line line chars - "\n\r\f\u0085\u2028\u2029")