HttpRequestStreamReader overrides for Read Span, ReadAsync Memory, ReadLine and ReadLineAsync #18802

alefranz · 2020-02-05T00:02:09Z

This is the corresponding change of #18451 (HttpResponseStreamWriter)

Currently the HttpRequestStreamReader relies on the base class implementation which cause a sync over async call.

This include a change on the sync implementation of Read(char[] buffer, int index, int count) to reuse the Span<char> implementation and consolidate the code, but I'm not sure if there are performance concern with this change.

Related to #2895 and #16740

src/Http/WebUtilities/src/HttpRequestStreamReader.cs

alefranz · 2020-02-17T00:39:28Z

Hi @jkotalik ,
I've updated the PR with a new implementation following your suggestion of using IndexOf instead of copying char by char. It also means that the StringBuilder is not necessary when the new line is already on the buffer, which lead to another nice performance boost.
Given it is better performing, I've overridden also the blocking ReadLine method.

It adds complexity to this class but given the performance boost should be worth it. I've also upped the test coverage to increase confidence on this addition.

I'm not really happy about the input/output parameters of the step method, especially the mixing of a return struct and ref parameters, but after some experiments this looked like the more readable version. Any suggestion?

About the benchmark, BaseReadLine is the ReadLine version from TextReader, while CreateReader is the overhead to create the reader as I wasn't able to leave it out of the benchmark (so you have to subtract that from each result).
The buffer length is the 1024 (default) and Length is the length of the data, with \r\n as the last 2 chars.
Essentially it is up to 5x faster and, when the new line is already on the buffer, does significantly less allocation.

Method	Toolchain	RunStrategy	Length	Mean	Error	StdDev	Op/s	Gen 0	Allocated
ReadLineAsync	.NET Core 5.0	Throughput	200	650.0 ns	7.414 ns	6.572 ns	1,538,475.3	0.0753	5960 B
ReadLine	.NET Core 5.0	Throughput	200	561.5 ns	7.087 ns	5.918 ns	1,780,810.8	0.0753	5744 B
BaseReadLine	.NET Core 5.0	Throughput	200	1,409.1 ns	9.703 ns	9.076 ns	709,686.2	0.0820	6616 B
CreateReader	.NET Core 5.0	Throughput	200	443.0 ns	3.507 ns	2.929 ns	2,257,188.1	0.0753	5320 B
ReadLineAsync	.NET Core 5.0	Throughput	1000	975.2 ns	4.934 ns	4.120 ns	1,025,434.2	0.0858	7560 B
ReadLine	.NET Core 5.0	Throughput	1000	866.2 ns	3.223 ns	2.517 ns	1,154,433.6	0.0916	7344 B
BaseReadLine	.NET Core 5.0	Throughput	1000	5,069.3 ns	70.659 ns	62.637 ns	197,267.6	0.1450	9896 B
CreateReader	.NET Core 5.0	Throughput	1000	447.0 ns	6.328 ns	5.919 ns	2,236,891.1	0.0753	5320 B
ReadLineAsync	.NET Core 5.0	Throughput	1025	1,380.4 ns	6.507 ns	5.434 ns	724,432.4	0.1507	9944 B
ReadLine	.NET Core 5.0	Throughput	1025	1,175.0 ns	11.773 ns	11.013 ns	851,070.1	0.1507	9584 B
BaseReadLine	.NET Core 5.0	Throughput	1025	4,685.1 ns	23.097 ns	20.475 ns	213,444.6	0.1450	9944 B
CreateReader	.NET Core 5.0	Throughput	1025	442.3 ns	5.487 ns	5.133 ns	2,260,706.0	0.0753	5320 B
ReadLineAsync	.NET Core 5.0	Throughput	1600	1,934.2 ns	37.282 ns	38.285 ns	517,008.5	0.1812	13288 B
ReadLine	.NET Core 5.0	Throughput	1600	1,710.8 ns	16.655 ns	13.907 ns	584,528.5	0.1812	12856 B
BaseReadLine	.NET Core 5.0	Throughput	1600	7,151.1 ns	30.762 ns	25.687 ns	139,838.1	0.1831	13216 B
CreateReader	.NET Core 5.0	Throughput	1600	443.6 ns	4.795 ns	4.485 ns	2,254,140.2	0.0753	5320 B

alefranz · 2020-03-02T13:37:38Z

Hi @jkotalik ,
I've fixed the reference assembly.
Have you had a chance to look at the benchmarks? is it enough?

alefranz · 2020-03-27T09:51:10Z

Hello! please let me know if I need to rebase this! thanks

jkotalik · 2020-03-30T19:45:03Z

src/Http/WebUtilities/src/HttpRequestStreamReader.cs

                _charBufferIndex += charsRemaining;

                charsRead += charsRemaining;
                count -= charsRemaining;

+                if (count > 0)


Why do we need to check count > 0?

jkotalik · 2020-03-30T19:46:15Z

src/Http/WebUtilities/src/HttpRequestStreamReader.cs


-                _charBufferIndex += n;
+                if (charsAvailable < count)


Why can't this be the same check done in Read with:

if (count > 0) { buffer = buffer.Slice(charsRemaining, count); }

aligned to Read. Also renamed charsAvailable to charsRemaining for simmetry.

jkotalik · 2020-03-30T19:49:12Z

src/Http/WebUtilities/src/HttpRequestStreamReader.cs

+                    return stepResult.Result ?? sb?.ToString();
+                }
+
+                continue;


nit: no need for continue.

jkotalik · 2020-03-30T19:54:55Z

src/Http/WebUtilities/src/HttpRequestStreamReader.cs

+        {
+            if (consumeLineFeed)
+            {
+                if (_charBuffer[_charBufferIndex] == '\n')


nit: I'd make \r and \n constants.

is it ok local to the method?

jkotalik

With some minor nits, LGTM.

alefranz

Thanks for the review. I've updated it to address the feedback.

alefranz · 2020-04-07T08:08:31Z

src/Http/WebUtilities/src/HttpRequestStreamReader.cs

                _charBufferIndex += charsRemaining;

                charsRead += charsRemaining;
                count -= charsRemaining;

+                if (count > 0)


alefranz · 2020-04-07T08:09:15Z

src/Http/WebUtilities/src/HttpRequestStreamReader.cs


-                _charBufferIndex += n;
+                if (charsAvailable < count)


aligned to Read. Also renamed charsAvailable to charsRemaining for simmetry.

alefranz · 2020-04-07T08:10:02Z

src/Http/WebUtilities/src/HttpRequestStreamReader.cs

+                    return stepResult.Result ?? sb?.ToString();
+                }
+
+                continue;


alefranz · 2020-04-07T08:12:52Z

src/Http/WebUtilities/src/HttpRequestStreamReader.cs

+        {
+            if (consumeLineFeed)
+            {
+                if (_charBuffer[_charBufferIndex] == '\n')


is it ok local to the method?

Tratcher · 2020-04-10T16:07:47Z

Compiler error? ##[error]HttpRequestStreamReaderReadLineBenchmark.cs(6,34): error CS0234: (NETCORE_ENGINEERING_TELEMETRY=Build) The type or namespace name 'Jobs' does not exist in the namespace 'BenchmarkDotNet.Attributes' (are you missing an assembly reference?)

alefranz · 2020-04-12T12:50:31Z

Compiler error? ##[error]HttpRequestStreamReaderReadLineBenchmark.cs(6,34): error CS0234: (NETCORE_ENGINEERING_TELEMETRY=Build) The type or namespace name 'Jobs' does not exist in the namespace 'BenchmarkDotNet.Attributes' (are you missing an assembly reference?)

Sorry @Tratcher , I didn't realize that the CI build the outcome of the merge on master and I guess the benchmark library has been updated on master.
I've fixed it now, let me know if you want me to properly rebase the fix or squash it, or it doesn't matter as it is going to be squash-merged.

Thank you,
Alessio

jkotalik · 2020-04-13T15:18:15Z

Thanks!

alefranz requested review from analogrelay, jkotalik and Tratcher as code owners February 5, 2020 00:02

ghost added the area-servers label Feb 5, 2020

jkotalik self-assigned this Feb 7, 2020

jkotalik reviewed Feb 9, 2020

View reviewed changes

src/Http/WebUtilities/src/HttpRequestStreamReader.cs Show resolved Hide resolved

jkotalik reviewed Feb 9, 2020

View reviewed changes

src/Http/WebUtilities/src/HttpRequestStreamReader.cs Outdated Show resolved Hide resolved

analogrelay mentioned this pull request Feb 11, 2020

Override Span and Memory Writes for HttpResponseStreamWriter #2895

Closed

analogrelay added this to the 5.0.0-preview1 milestone Feb 11, 2020

alefranz changed the title ~~HttpRequestStreamReader overrides for Read Span, ReadAsync Memory and ReadLineAsync~~ HttpRequestStreamReader overrides for Read Span, ReadAsync Memory, ReadLine and ReadLineAsync Feb 17, 2020

analogrelay modified the milestones: 5.0.0-preview1, 5.0.0-preview2 Feb 19, 2020

jkotalik reviewed Mar 30, 2020

View reviewed changes

jkotalik approved these changes Mar 30, 2020

View reviewed changes

alefranz commented Apr 7, 2020

View reviewed changes

alefranz added 8 commits April 12, 2020 09:26

Add Read Span<char> overload to HttpRequestStreamReader

b21ca72

ReadAsync for Memory of char override

5318b1f

Refactored Read Span and added tests for dispose

ecae875

Implemented ReadLineAsync

0229019

Updated reference assembly

cd7538b

ReadLine reimplementation

e3b3db3

Updated reference assemblies

e48e7e1

Feedback

327aa1f

alefranz force-pushed the HttpRequestStreamReader-Span-Memory branch from 23b6f74 to 327aa1f Compare April 12, 2020 08:26

Fixed benchmark for master changes

5c3a347

jkotalik merged commit e3d3da3 into dotnet:master Apr 13, 2020

jkotalik mentioned this pull request Apr 13, 2020

Implement HttpRequestStreamReader.ReadToEndAsync #18232

Merged

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023

HttpRequestStreamReader overrides for Read Span, ReadAsync Memory, ReadLine and ReadLineAsync #18802

HttpRequestStreamReader overrides for Read Span, ReadAsync Memory, ReadLine and ReadLineAsync #18802

Uh oh!

Conversation

alefranz commented Feb 5, 2020

Uh oh!

Uh oh!

Uh oh!

alefranz commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alefranz commented Mar 2, 2020

Uh oh!

alefranz commented Mar 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotalik left a comment

Choose a reason for hiding this comment

Uh oh!

alefranz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Apr 10, 2020

Uh oh!

alefranz commented Apr 12, 2020

Uh oh!

jkotalik commented Apr 13, 2020

Uh oh!

Uh oh!

alefranz commented Feb 17, 2020 •

edited

Loading