Skip to content

StreamReader sync/async optimizations: ArrayPool, IndexOfAny, ValueStringBuilder #62552

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.

Sign up for GitHub

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

Closed
wants to merge 26 commits into from
Closed

StreamReader sync/async optimizations: ArrayPool, IndexOfAny, ValueStringBuilder #62552

Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
ccbec0f
Store string-builder as a field and clear when reuse.
Dec 8, 2021
d340760
IndexOfAny
Dec 8, 2021
e2e318f
Fixes by review.
Dec 8, 2021
7ac2d06
Added array pools as a draft.
Dec 9, 2021
ff0ee6d
Return new string from polled array with specific length.
Dec 9, 2021
6f2af2d
Add ValueStringBuilder and fix length in ReadAsyncInternal.
Dec 10, 2021
03fae09
Summing char-position
Dec 10, 2021
75cfdc6
Added tests for the async version of ReadLine
Dec 10, 2021
04860a6
Renaming
Dec 10, 2021
7d338a1
ReadToEndAsyncInternal to ArrayPools.
Dec 10, 2021
b8a91ff
PR comments fixes and a hack for CI
Dec 11, 2021
d490b85
Handle exceptions.
Dec 11, 2021
576e953
AsSpan with specified length
Dec 11, 2021
ba913a9
Remove commented out StringBuilder
Dec 11, 2021
77a5827
Merge branch 'main' of github.com:dotnet/runtime into optimizations/7…
Jan 31, 2022
4d46850
Merge branch 'main' into optimizations/7.0.0/stream-reader
Trapov Mar 22, 2022
6d81c3b
Fix merge conflicts
May 11, 2022
dc93cbe
Merge branch 'optimizations/7.0.0/stream-reader' of github.com:Trapov…
May 11, 2022
7526cad
Remove try catch block
May 11, 2022
ec33696
Fixed issues
May 11, 2022
8060dc2
Fix merging issues
May 11, 2022
554ca18
Remove merging issues x2
May 11, 2022
c6d3eea
Changes after review
May 11, 2022
6325e8e
ReadToEnd revert
May 11, 2022
9922cf8
Fix-up
May 11, 2022
cc41cb2
Merge remote-tracking branch 'origin/main' into optimizations/7.0.0/s…
Jul 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 76 additions & 61 deletions src/libraries/System.Private.CoreLib/src/System/IO/StreamReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Buffers;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Text;
Expand Down Expand Up @@ -788,43 +790,39 @@ private int ReadBuffer(Span<char> userBuffer, out bool readToUserBuffer)
}
}

StringBuilder? sb = null;
using ValueStringBuilder sb = new(stackalloc char[256]);
do
{
int i = _charPos;
do
// Note the following common line feed chars:
// \n - UNIX/Mac \r\n - Windows
int i = _charBuffer.AsSpan(_charPos, _charLen - _charPos).IndexOfAny('\r', '\n');
if (i >= 0)
{
char ch = _charBuffer[i];
// Note the following common line feed chars:
// \n - UNIX \r\n - DOS \r - Mac
if (ch == '\r' || ch == '\n')
string s;
if (sb.Length > 0)
{
string s;
if (sb != null)
{
sb.Append(_charBuffer, _charPos, i - _charPos);
s = sb.ToString();
}
else
{
s = new string(_charBuffer, _charPos, i - _charPos);
}
_charPos = i + 1;
if (ch == '\r' && (_charPos < _charLen || ReadBuffer() > 0))
sb.Append(_charBuffer.AsSpan(_charPos, i));
s = sb.ToString();
}
else
{
s = new string(_charBuffer, _charPos, i);
}
char ch = _charBuffer[_charPos + i];
_charPos += i + 1;
if (ch == '\r' && (_charPos < _charLen || ReadBuffer() > 0))
{
if (_charBuffer[_charPos] == '\n')
{
if (_charBuffer[_charPos] == '\n')
{
_charPos++;
}
_charPos++;
}
return s;
}
i++;
} while (i < _charLen);
return s;
}

i = _charLen - _charPos;
sb ??= new StringBuilder(i + 80);
sb.Append(_charBuffer, _charPos, i);

sb.Append(_charBuffer.AsSpan(_charPos, i));
} while (ReadBuffer() > 0);
return sb.ToString();
}
Expand Down Expand Up @@ -881,63 +879,80 @@ private int ReadBuffer(Span<char> userBuffer, out bool readToUserBuffer)

private async Task<string?> ReadLineAsyncInternal(CancellationToken cancellationToken)
{
if (_charPos == _charLen && (await ReadBufferAsync(cancellationToken).ConfigureAwait(false)) == 0)
static char[] ResizeOrPoolNewArray(char[]? array, int atLeastSpace)
{
return null;
if (array == null)
return ArrayPool<char>.Shared.Rent(atLeastSpace);

char[] newArr = ArrayPool<char>.Shared.Rent(array.Length + atLeastSpace);
Array.Copy(array, newArr, array.Length);
ArrayPool<char>.Shared.Return(array);
return newArr;
}
static void Append(ref char[]? to, int destinationOffset, char[] @from, int offset, int length)
{
if (to == null || (destinationOffset + length) > to.Length)
{
to = ResizeOrPoolNewArray(to, destinationOffset + length + 80);
}

StringBuilder? sb = null;
Array.Copy(@from, offset, to, destinationOffset, length);
}

do
if (_charPos == _charLen && (await ReadBufferAsync(cancellationToken).ConfigureAwait(false)) == 0)
{
char[] tmpCharBuffer = _charBuffer;
int tmpCharLen = _charLen;
int tmpCharPos = _charPos;
int i = tmpCharPos;
return null;
}

char[]? rentedArray = null;
int lastWrittenIndex = 0;
try
{
do
{
char ch = tmpCharBuffer[i];

// Note the following common line feed chars:
// \n - UNIX \r\n - DOS \r - Mac
if (ch == '\r' || ch == '\n')
// \n - UNIX/Mac \r\n - Windows
int i = _charBuffer.AsSpan(_charPos, _charLen - _charPos).IndexOfAny('\r', '\n');
if (i >= 0)
{
string s;

if (sb != null)
string? s;
if (rentedArray != null)
{
sb.Append(tmpCharBuffer, tmpCharPos, i - tmpCharPos);
s = sb.ToString();
Append(ref rentedArray, lastWrittenIndex, _charBuffer, _charPos, i);
lastWrittenIndex += i;
s = new string(rentedArray!, 0, lastWrittenIndex);
}
else
{
s = new string(tmpCharBuffer, tmpCharPos, i - tmpCharPos);
s = new string(_charBuffer, _charPos, i);
}

_charPos = tmpCharPos = i + 1;

if (ch == '\r' && (tmpCharPos < tmpCharLen || (await ReadBufferAsync(cancellationToken).ConfigureAwait(false)) > 0))
char ch = _charBuffer[_charPos + i];
_charPos += i + 1;
if (ch == '\r' && (_charPos < _charLen || (await ReadBufferAsync(cancellationToken).ConfigureAwait(false)) > 0))
{
tmpCharPos = _charPos;
if (_charBuffer[tmpCharPos] == '\n')
if (_charBuffer[_charPos] == '\n')
{
_charPos = ++tmpCharPos;
_charPos++;
}
}

return s;
}

i++;
} while (i < tmpCharLen);

i = tmpCharLen - tmpCharPos;
sb ??= new StringBuilder(i + 80);
sb.Append(tmpCharBuffer, tmpCharPos, i);
} while (await ReadBufferAsync(cancellationToken).ConfigureAwait(false) > 0);
i = _charLen - _charPos;

return sb.ToString();
Append(ref rentedArray, lastWrittenIndex, _charBuffer, _charPos, i);
lastWrittenIndex += i;
} while (await ReadBufferAsync(cancellationToken).ConfigureAwait(false) > 0);
return new string(rentedArray!, 0, lastWrittenIndex);
}
finally
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a general rule, I don't believe we generally bother with a try/finally simply to return a pooled array. Hopefully exceptions are exceptional, and in this case failing to return an array is a small issue.
The key rules are to return it in the normal case, to only return it once, and to not use it after returning.

Copy link
Author

@Trapov Trapov May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If something goes wrong then we wouldn’t be able to return to the shared pool. I don’t know but it seems like an issue to me, that’s why have a try/finally block. What do you suggest?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct, but if something goes wrong then we have a bigger problem than wasting an allocation. And so we prefer the simpler code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really have a problem if a cancellation token triggered on an IO operation (ReadBufferAsync)? That's like a valid case where we wouldn't return to the shared pool (if we omit the finally block) but continue with the normal execution.

I feel like I don't understand the suggestion or what you mean, sorry :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The situation we're trying to avoid is something holding on to the buffer after the exception is thrown and returning into the pool prematurely. Lets say for example ReadBufferAsync failed in a way where the buffer was still being used by a background thread (a rogue Task.Run). This finally would run, return to the pool, then another piece of code would rent it and it would still be in used by the rogue Task.Run. To count this out, we don't bother returning to the pool in exceptional cases.

Copy link
Author

@Trapov Trapov May 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I get it. But still ReadBufferAsync doesn't use our rented array and it's being rented and used only in this place so no rogue task could use it since it doesn't get forwarded to any of the functions down to call-stack. @davidfowl

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that it makes the code cleaner to not have the try/ finally. ArrayPool is all about stats, we’re assuming there will be far far more successes here than exceptions.
Also try/finally are not necessarily free: they may impede some codegen optimizations (eg inlining, although perhaps not relevant here)

{
if (rentedArray != null)
{
ArrayPool<char>.Shared.Return(rentedArray);
}
}
}

public override Task<string> ReadToEndAsync() => ReadToEndAsync(default);
Expand Down