ResponseCaching: started conversion to pipes

[alefranz]

I've started to look into migrating the ResponseCaching middleware to pipes.

For now this only use pipes to write the response from the cache.
This already allow to remove some code.
It also remove a memory copy when saving to the cache, however I do not understand for which scenario it was needed before (and so it would be still needed now) or if it was just a consequence of the design.

Migrating the capturing of the response to cache, getting read of the stream implementation, is more tricky and I am not sure what it would be a correct design.
Currently it rely on replacing the Body with a shim of the response stream that capture all the writes and duplicate them to buffer, however it is not possible to replace the BodyWriter instead.
Also, should not the shim be applied on the IHttpResponseBodyFeature, like it is done in the ResponseCompression middleware?
I imagine would make sense to have a common design for both middlewares.

I will keep looking at this, but I opened this draft PR so if you already have a design in mind we can discuss it.
I don't think merging only this part makes sense.

Related #11377

[gfoidl]

Didn't look at the overall design, just these nits.

[alefranz]

I had another look at this and it looks like the pipe writer is built around the stream, so keep wrapping the original stream doesn't give a performance disadvantage.
What am I missing?

[analogrelay]

@jkotalik can you take a look at this and see what you think of the design so we know how to move forward?

[jkotalik]

It also remove a memory copy when saving to the cache, however I do not understand for which scenario it was needed before (and so it would be still needed now) or if it was just a consequence of the design.

From what I can tell, what you did looks like strictly an improvement.

Migrating the capturing of the response to cache, getting read of the stream implementation, is more tricky and I am not sure what it would be a correct design.
Currently it rely on replacing the Body with a shim of the response stream that capture all the writes and duplicate them to buffer, however it is not possible to replace the BodyWriter instead.

It definitely is possible to replace the BodyWriter instead, however I can see that there would be some difficulties. From what I can tell, when someone calls WriteAsync, the stream shim calls FinalizeCacheHeaders, which sets some headers on the response (see https://github.com/aspnet/AspNetCore/blob/e659b55c33b20a92737a43427b60c5c73375ae5b/src/Middleware/ResponseCaching/src/ResponseCachingMiddleware.cs#L254). The difficulties with the BodyWriter will be that when someone calls GetMemory/GetSpan/Advance to write part of the response body, then call FlushAsync(), the headers need to be written to the response before writing the response body itself. If someone wrote content to GetMemory, we would somehow need to inject the headers before. The only way to do this is saving all writes before first flush and then make the pipe passthrough afterwards.

We solved this in kestrel itself on the original PipeWriter by doing internal tracking/leasing: https://github.com/aspnet/AspNetCore/blob/master/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs#L207. This logic is fairly straight forward; you should be able to use a lot of what exists there.

I don't know it's worth shimming the response stream with a pipe here though, the added complexity it introduces will make it hard to maintain and I don't know how much of a perf benefit you'd get for it.

I think the current change looks like something we would take though.

[jkotalik]

Was read ever used? Otherwise, makes sense.

[alefranz]

SegmentReadStream was used to wrap the data stored in the cache, so Read was using when serving from cache.
Now I'm using CachedResponseBody just as a wrapper of the byte segments, which then copy to the pipewriter without using a stream, so this is no longer needed.
Is that what you were asking?

[alefranz]

It definitely is possible to replace the BodyWriter instead, however I can see that there would be some difficulties.

Hi @jkotalik ,
it would also require to make to change the HttpResponse to be able to replace the BodyWriter which currently is a readonly property.
Do you know if there is any benchmark I can use as a base to find out if there could be a significant performance benefit in not using the stream?

About this PR, I've rebased it and addressed the feedback.
I will mark it for review once I'm done with the tests.

[alefranz]

I've also added a benchmark and it seems like slightly slower then before although I'm not sure the benchmark is really representative and the error margin is quite high,

Before:

Method	Mean	Error	StdDev	Median	Op/s	Gen 0	Allocated
Cache	11.307 us	0.1968 us	0.4562 us	11.492 us	88,443.0	-	4.73 KB
ServeFromCache	2.884 us	0.0157 us	0.0139 us	2.882 us	346,758.8	0.0267	2.37 KB

After:

Method	Mean	Error	StdDev	Op/s	Gen 0	Allocated
Cache	12.086 us	1.5640 us	4.5375 us	82,737.1	-	4.16 KB
ServeFromCache	3.206 us	0.0259 us	0.0216 us	311,938.1	0.0267	2.45 KB

[analogrelay]

I've also added a benchmark and it seems like slightly slower then before

Hmmmmmmmm... that's not ideal. Given that we're already a little unsure of the value of this change I'm not sure it's worth it right now.

[alefranz]

I've reworked the benchmark, using an actual Pipe which should be more representative of the behavior with Kestrel.
I've also added different response sizes.
Now it results in a speedup and less allocation except for really small payloads of a few bytes.
I've also added a large payload (1MB) even if the default limit is 64KB.

It would be great if someone can review the benchmark to confirm the validity of these results:
https://github.com/aspnet/AspNetCore/pull/16961/files#diff-3ec34e95e14d0484ca182d967bb33d4c

Before:

Method	Size	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
Cache	100	15.897 us	0.3242 us	0.9353 us	62,903.5	0.0610	-	-	6.06 KB
ServeFromCache	100	6.839 us	0.1818 us	0.5333 us	146,213.5	0.0381	-	-	3.08 KB
Cache	65536	85.582 us	1.6837 us	1.2174 us	11,684.6	4.2725	0.6104	-	314.45 KB
ServeFromCache	65536	107.485 us	2.0812 us	2.3133 us	9,303.6	-	-	-	5.12 KB
Cache	1048576	1,190.080 us	23.0963 us	31.6145 us	840.3	46.8750	25.3906	3.9063	841.38 KB
ServeFromCache	1048576	296.630 us	5.8470 us	5.7425 us	3,371.2	-	-	-	5.46 KB

After:

Method	Size	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
Cache	100	16.590 us	0.3300 us	0.8280 us	60,279.1	0.0610	-	-	5.4 KB
ServeFromCache	100	7.091 us	0.1401 us	0.3133 us	141,015.8	0.0305	-	-	2.97 KB
Cache	65536	72.965 us	1.4504 us	2.7242 us	13,705.2	2.6855	0.4883	-	188.16 KB
ServeFromCache	65536	88.289 us	1.9925 us	2.5199 us	11,326.5	-	-	-	3.61 KB
Cache	1048576	934.144 us	18.6188 us	19.1201 us	1,070.5	31.2500	17.5781	3.9063	832.39 KB
ServeFromCache	1048576	249.305 us	4.4989 us	3.9881 us	4,011.1	-	-	-	3.61 KB

[analogrelay]

Next steps: @JunTaoLuo 's going to clone and take a look and do some checking here, as well as checking the benchmark validity.

We'll also need to rebase ;). @alefranz can you do that?

The benchmark results look OK to me, so it seems like it's worth taking if only so that we use more pipes.

[alefranz]

HI @anurse , @JunTaoLuo ,

I've rebased the PR.
Please let me know if there is anything I can help with.

Thank you!

[JunTaoLuo]

Changes look good. I'm a fan of the simplification and the benefit with large responses. I agree that we don't need to worry about the response stream for now so this will be ready to merge once rebased.

[alefranz]

Addressed feedback and rebased.
It should be good to merge now.

Thank you!

[JunTaoLuo]

Looks like there's a test failure:

A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:01.41]     Microsoft.AspNetCore.ResponseCaching.Tests.CachedResponseBodyTests.Copy_DoNothingWhenNoSegments [FAIL]
  X Microsoft.AspNetCore.ResponseCaching.Tests.CachedResponseBodyTests.Copy_DoNothingWhenNoSegments [523ms]
  Error Message:
   System.OperationCanceledException : The operation was canceled.
  Stack Trace:
     at System.Threading.CancellationToken.ThrowOperationCanceledException()
   at System.Threading.CancellationToken.ThrowIfCancellationRequested()
   at System.IO.Pipelines.Pipe.GetReadAsyncResult()
   at System.IO.Pipelines.Pipe.DefaultPipeReader.GetResult(Int16 token)
   at Microsoft.AspNetCore.ResponseCaching.Tests.CachedResponseBodyTests.ReceiveDataAsync(PipeReader reader, List`1 receivedSegments, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.ResponseCaching.Tests.CachedResponseBodyTests.Copy_DoNothingWhenNoSegments() in /_/src/Middleware/ResponseCaching/test/CachedResponseBodyTests.cs:line 52
--- End of stack trace from previous location ---
Results File: /datadisks/disk1/work/A9BF0946/w/B1FF0983/e/TestResults/_c001LHN_2020-01-12_13_45_39.trx

Given that this is failing in one out of many runs, it's likely that it's flaky.

[JunTaoLuo]

@alefranz can you look into fixing the flaky test?

[alefranz]

Hi John, apologies for the delay.
I'm confident that the issue was caused by a timeout too short, which I have now increased.

[JunTaoLuo]

Thanks for the contribution!

[alefranz]

Thank you for your help!