5
\$\begingroup\$

This code is a Extension to just be able to read a line quickly and efficiently with support for SslStream.

What i'm looking to achieve:

  • Lower CPU usage by improving how it stores bytes for being converted to string later
  • Lower CPU usage by possibly not reading per-byte some how. In my debug tests its using up the most CPU by far. 33%/55% of samples are the ReadByte

So your probably asking why im using a Queue as a buffer? Basically the Queue allows me to more efficiently store bytes in an array format. byte[] is un-acceptable due to the fact its size can't be changed dynamically (you would have to make a new byte, move over the old data, add the new, replace the old byte[], in-efficient)

queue however (or Stack if you .Reverse() it with linq) lets you add bytes whenever you want and then .ToArray when you need it as a byte[] Same thing can be done with List<byte> but I dont feel like it's the best tool for the job in this case.

Another thing I could do here is just convert with Encoding.GetString on every byte and StringBuilder it but that seems to use more CPU and a lot more memory.

Any suggestions?

public static string ReadLine(this Stream stream, ref int bodySize, Encoding encoding) {
    bool bodySizeWasSpecified = bodySize > 0;
    byte b = 0;
    Queue<byte> Buffer = new Queue<byte>();
    while (true) {
        #region Try Get 1 Byte from Stream
        try {
            int i = stream.ReadByte();
            if (i == -1) {
                break;//stream ended/closed
            }
            b = (byte)i;
        } catch {
            return null;//timeout //not authenticated context
        }
        #endregion
        #region If there's a body size specified, decrement back 1
        if (bodySizeWasSpecified) {
            bodySize--;
        }
        #endregion
        #region If Byte is \n or \r
        if (b == 10 || b == 13) {
            #region If ByteArray is Empty and the byte is \n reloop so we dont start with a leading \n
            if (Buffer.Count == 0 && b == 10) {
                continue;
            }
            #endregion
            #region We hit a newline, lets finish the reads here.
            break;
            #endregion
        }
        #endregion
        #region Add the read byte to the Byte Array
        Buffer.Enqueue(b);
        #endregion
        #region Break if bodysize was greater than 0 but now its 0
        if (bodySizeWasSpecified && bodySize == 0) {
            break;
        }
        #endregion
    }
    return encoding.GetString(Buffer.ToArray());
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I need to ask you to roll back your last edits. Please see What should I do when someone answers my question?: Do not add an improved version of the code after receiving an answer. Including revised versions of the code makes the question confusing, especially if someone later reviews the newer code. \$\endgroup\$ – t3chb0t Oct 30 '18 at 18:32
  • \$\begingroup\$ You put a lot of effort in editing the question so I wouldn't like to just undo it but please remove all the edits and put them in another answer.... don't be surprised if someone will revert it for you... \$\endgroup\$ – t3chb0t Oct 30 '18 at 18:37
  • \$\begingroup\$ @t3chb0t darn alright, reverted :( \$\endgroup\$ – Ma Dude Oct 30 '18 at 18:42
  • \$\begingroup\$ You could have just copy/pasted it into a self-answer - it was quite interesting ;-( \$\endgroup\$ – t3chb0t Oct 30 '18 at 18:53
4
\$\begingroup\$

So your probably asking why im using a Queue as a buffer? Basically the Queue allows me to more efficiently store bytes in an array format. byte[] is un-acceptable due to the fact its size can't be changed dynamically (you would have to make a new byte, move over the old data, add the new, replace the old byte[], in-efficient)

Well if you read the docs for Stream.ReadByte() you would see this

Notes to Inheritors

The default implementation on Stream creates a new single-byte array and then calls Read(Byte[], Int32, Int32). While this is formally correct, it is inefficient. Any stream with an internal buffer should override this method and provide a much more efficient version that reads the buffer directly, avoiding the extra array allocation on every call.

and if we dig into the reference source of Stream we would stumble over the default implementation

public abstract int Read([In, Out] byte[] buffer, int offset, int count);

// Reads one byte from the stream by calling Read(byte[], int, int). 
// Will return an unsigned byte cast to an int or -1 on end of stream.
// This implementation does not perform well because it allocates a new
// byte[] each time you call it, and should be overridden by any 
// subclass that maintains an internal buffer.  Then, it can help perf
// significantly for people who are reading one byte at a time.
public virtual int ReadByte()
{
    Contract.Ensures(Contract.Result<int>() >= -1);
    Contract.Ensures(Contract.Result<int>() < 256);

    byte[] oneByteArray = new byte[1];
    int r = Read(oneByteArray, 0, 1);
    if (r==0)
        return -1;
    return oneByteArray[0];
}  

which isn't overwritten neither by NetworkStream nor by AuthenticatedStream (which is inherited by SslStream).

That beeing said, let's dig into the code...

Region's are considered as an antipattern especially if they occur inside a method. Take a look at Are #regions an antipattern or code smell?

Take e.g

    #region If Byte is \n or \r
    if (b == 10 || b == 13)
    {  

this should be replaced by either

    if (b == '\n' || b == '\r')
    {  

or by

    const char NewLine = '\n';
    const char Return = '\r';

    if (b == NewLine || b == Return)
    {

I don't really like the catch here because it isn't specific. You should consider to remove the try..catch completely and let the calling code handle any thrown exception. This leads to another problem: The method is public but you don't validate if stream is null. You will just let the exception be thrown and catched by the catch I mentioned.


I would add two overloaded ReadLine() methods to distinguish between needing the bodySize and let these methods return an IEnumerable<byte> which I would then convert to a byte[]. The methods would be called like so

 public static string ReadLine(this Stream stream, ref int bodySize, Encoding encoding)
{
    if (stream == null) { throw new ArgumentNullException("stream"); }

    byte[] result;

    if (bodySize > 0)
    {
        result = stream.ReadLine(bodySize).ToArray();
        bodySize -= result.Length;
    }
    else
    {
        result = stream.ReadLine().ToArray();
    }

    return encoding.GetString(result);

}

which is much easier to read.

The methods themself could look like so

private const char NewLine = '\n';
private const char Return = '\r';

private static IEnumerable<byte> ReadLine(this Stream stream, int bodySize)
{
    int i;
    int counter = 0;
    while (bodySize > 0 && (i = stream.ReadByte()) > 0)
    {
        byte b = (byte)i;
        if (b == NewLine || b == Return)
        {
            // Break only if it isn't the first byte.
            if (counter > 0 || b != NewLine) { yield break; }

            continue;

        }
        counter++;
        bodySize--;
        yield return b;
    }
}

private static IEnumerable<byte> ReadLine(this Stream stream)
{
    int i;
    int counter = 0;
    while ((i = stream.ReadByte()) > 0)
    {
        byte b = (byte)i;
        if (b == NewLine || b == Return)
        {
            // Break only if it isn't the first byte.
            if (counter > 0 || b != NewLine) { yield break; }

            continue;

        }
        counter++;
        yield return b;
    }
}  

or if we want to be clever we could change the second method like so

private static IEnumerable<byte> ReadLine(this Stream stream)
{
    return stream.ReadLine(int.MaxValue);
}

Please note that the provided code isn't tested.

\$\endgroup\$
  • \$\begingroup\$ @HenrikHansen Well, one shouldn't remove regions and not add a comment if one is necessary. If the first byte equals ' \n' the loop should continue. Added the comment. Thanks. \$\endgroup\$ – Heslacher Oct 30 '18 at 8:01
  • \$\begingroup\$ @HenrikHansen you aren't blind, its me beeing stupid! \$\endgroup\$ – Heslacher Oct 30 '18 at 8:05
  • \$\begingroup\$ Hi! Appreciate a lot of stuff you showed me here. But couple questions. Does using a constant char for the \n and \r actually pose any benefit or just "readability" because I actually prefer seeing \n and \r. Also for the (i = stream.ReadByte()) whats the best way to handle the exception? Or should I handle it by the calling code as you said? Also you mentioned about MSDN notes on the .ReadByte() saying it would be best to make an override method of it and handle it directly, could you give me a PoC of that? \$\endgroup\$ – Ma Dude Oct 30 '18 at 8:19
  • \$\begingroup\$ The constants adds readability which is a great benefit. The exception handling should be done by the calling code. For the ReadByte() this should be done if your class would inherit/extend the abstract Stream class. \$\endgroup\$ – Heslacher Oct 30 '18 at 8:28
3
\$\begingroup\$

Design

  • First of all, is there a particular reason why you're not using a StreamReader?
  • Reading one byte at a time is a bad idea if you care about performance, but if you read multiple bytes at once there's a risk of reading too much, and not all streams support reverting to a previous position. You'll want to read into a buffer for performance sake, but you don't want to throw away what's left in that buffer. That's why StreamReader is a class that wraps a given stream, instead of a static, stateless (extension) method.
  • Judging by the name and first argument type this looks like a general-purpose, reusable method (reading lines from any kind of stream), but it also contains a few very specific notes (timeout, not authenticated context). If it's meant to be used only in a specific context then I wouldn't make it a public extension method.
  • Your method returns null when the given stream throws an exception, and it returns an empty string when the end of the stream has been reached. Not only is that inconsistent with the similarly named StreamReader.ReadLine method, it also hides problems from the caller by gobbling up exceptions, and it doesn't let the caller differentiate between an actual empty line and the end of the stream. This behavior isn't documented either (which is especially important for general-purpose reusable methods).

Performance

  • When it comes to performance, always measure! I ran a few tests against code that's using a StreamReader, and the code you've shown here is quite a bit slower. Reading one byte at a time incurs a lot more call overhead, if not anything else, and it shows.
  • If you know how much data you need, then it's best to allocate a buffer up-front. However, keep in mind that the number of bytes isn't necessarily equal to the number of characters, depending on encoding and actual string content.
  • A Queue or List can be more convenient than an array, but they're still using arrays internally, and as you're adding items they'll occasionally need to allocate a larger array to make room for new items, so they're not necessarily more efficient. Also, several Stream and Encoding methods contain overloads that work with arrays and offset/counts, so you're often better off with arrays.
  • Minor point: writing to a ref variable is slightly slower than writing to a local variable. It's not clear to me why bodySize even needs to be passed by ref?

Other notes

  • Heslacher already pointed to a thread that shows why regions are considered an anti-pattern. You seem to be using them as comments - so why not just use actual comments instead? However, quite a few of them are just repeating what the code already says, so they're just wasting space. In general, I'd only add comments to explain why code does what it does or how it's meant to be used. What it does should usually be clear from the code itself.
  • bodySize is not a very descriptive name, and not a name I'd expect in a general-purpose method. I'd rename this to something like maxLength or maxLineLength.
  • Edge-case: if bodySize is more than 0, and if the given stream contains more leading '\n' characters than bodySize, then this method ignores bodySize and returns the first full line after those newline characters, with bodySize ending up being negative.
\$\endgroup\$
  • \$\begingroup\$ StreamReader's ReadLine method works differently on how it checks for a new-line, which is why I use a custom method. It's not really a general purpose extension, mainly geared towards SslStream that wraps NetworkStream from a TcpClient. It catches the exception in the method directly as all code that calls ReadLine relatively do the exact same thing. So I just null check it, less code over-all, same performance. \$\endgroup\$ – Ma Dude Oct 30 '18 at 18:10
  • \$\begingroup\$ Also is StreamReader.ReadLine really anymore efficient, by looking at reference source it seems to be relatively similar, apart from running an internal method called ReadBuffer() which im assuming does similar as ReadByte(). \$\endgroup\$ – Ma Dude Oct 30 '18 at 18:28
  • \$\begingroup\$ If it's such a specific method then I'd make it an internal utility method and clearly document its behavior and intended use. I'd also think twice about that exception handling - right now you won't know what went wrong when an unexpected problem occurs. As for ReadBuffer, try for yourself - in almost all cases it should be significantly faster, assuming you've chosen an appropriate buffer size. \$\endgroup\$ – Pieter Witvoet Oct 30 '18 at 22:25
2
\$\begingroup\$

When I test your method with a file stream, it runs forever, because there is no real stop condition. When EOF is reached it just keeps returning an empty string. You should return null or signal otherwise to the client that no more lines was found.

You can maybe do:

  ...
  if (stream.Position >= stream.Length)
    return null;

  return encoding.GetString(Buffer.ToArray());
}

you can't just check the size of the Buffer because a line actually can be empty.


Another consideration I would make is the definition of a new line is in the current environment/context. It can be tricky because you maybe receive data from other environments.

.NET provides Environment.NewLine that defines the current environments new line sequence.

If you have a sequence as AAAA\n\n\r\nBBBBB - how many lines is that? I would say 4 ("AAA, "", "", "BBB") but your method gives 3.


If you need a ReadLine() method, you probably need to read more than just one line. I would consider to implement a ReadLines() method instead:

public static IEnumerable<string> ReadLines(this Stream stream, Encoding encoding = null)
{
  // TODO: the actual implementation...
  encoding = encoding ?? Encoding.UTF8;
  int bytesRead = 0;
  while ((bytesRead = stream.Read(buffer, 0, bufferSize)) > 0)
  {
    if (new line found)
      yield return encoding.GetString(resultBuffer.ToArray());
    ...
  }
}

You can in this way read larger chunks of bytes from the stream and buffer them in a much more efficient way than when reading one byte at a time.

\$\endgroup\$
  • \$\begingroup\$ Stream.Read is the way to go, but I'd be wary of using a 'lazy' approach here. Consider code like foreach (var line in stream.ReadLines()) { if (...) break; } or stream.ReadLines().FirstOrDefault() - seems fine, but whatever data is left in the buffer after returning the last line is lost. \$\endgroup\$ – Pieter Witvoet Oct 31 '18 at 12:30
  • \$\begingroup\$ @PieterWitvoet: You do have a point. I just thought that OP probably wanted to read all the lines at once when receiving the SslStream and in that case, it seems OK to me. Alternatively you could extract all lines at once and return them as a materialized collection (array, list etc.) and name the method ReadAllLines() \$\endgroup\$ – Henrik Hansen Oct 31 '18 at 12:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.