Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

avoid async overhead in ReadNextLineAsync#23329

Closed
geoffkizer wants to merge 2 commits into
dotnet:masterfrom
geoffkizer:readnextline2
Closed

avoid async overhead in ReadNextLineAsync#23329
geoffkizer wants to merge 2 commits into
dotnet:masterfrom
geoffkizer:readnextline2

Conversation

@geoffkizer
Copy link
Copy Markdown

Updated version of #23210

@stephentoub

I'm blocked on getting perf data here for now, but I expect it to be similar to the previous PR.

int length = _readLength - startIndex;
int crPos = Array.IndexOf(_readBuffer, (byte)'\r', startIndex, length);
if (crPos < 0)
await FillAsync(cancellationToken);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

.ConfigureAwait(false)

{
while (true)
{
await FillAsync(cancellationToken);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

.ConfigureAwait(false)

Copy link
Copy Markdown
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Other than my comments, as long as perf looks good, LGTM.

@karelz
Copy link
Copy Markdown
Member

karelz commented Sep 6, 2017

@geoffkizer what is the status of this PR? It's almost 3 weeks old now ...

@karelz
Copy link
Copy Markdown
Member

karelz commented Oct 2, 2017

@geoffkizer ping?

@karelz karelz added this to the 2.1.0 milestone Oct 28, 2017
@karelz
Copy link
Copy Markdown
Member

karelz commented Nov 20, 2017

@geoffkizer any update?

@stephentoub
Copy link
Copy Markdown
Member

@geoffkizer, did you want to move ahead with this PR? Or should we close it for now?

@karelz
Copy link
Copy Markdown
Member

karelz commented Nov 30, 2017

Closing for now. We can reopen later, when it is ready.

@karelz karelz closed this Nov 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants