Spiked HttpResponsePipeWriter #22916

alefranz · 2020-06-13T18:01:21Z

Hello!

This is an experiment related to #7836 which I spiked a while back as mentioned on #7836 (comment)

This introduces a HttpResponsePipeWriter to write encoded to a PipeWriter instead of a Stream, to be used by MVC including Razor views.
Please note that the buffering is not implemented yet and it is writing directly to pipe, instead of buffering in memory (and spooling to file)

I'm opening this draft PR to see if there is an interest for this change and in the hope of gathering feedback to understand what would be the right approach for this feature.
The goal is also to understand what changes would be required in terms of public API surface, and eventually create an issue to discuss them later.

I've failed a separate issue as I am currently unable to run the benchmark #22915

Thank you,
Alessio

rynowak · 2020-06-14T22:18:06Z

@davidfowl @pranavkm @Tratcher - Alessio was looking for a bigger project to contribute for ASP.NET Core and I suggested that he look at the Razor rendering process, and removing some of the layers it has.

One of those layers this this boi - which basically functions like a PipeWriter but delegates to a TextWriter instead of a pipe. Now that we have pipes in Kestrel, we can remove this and eliminate an extra round of buffering, and a usage of the memory pool. Introducing this writer is a good first step because it can pretty much immediately be used by MVC for an improvement.

The pipe model of I/O is better for Razor because IHtmlContent is synchronous. Razor accumulates a big list of 'write operations' (union IHtmlContent | string), but the ultimately I/O context is async. So we can acquire memory, write an IHtmlContent to it synchronously, and then asynchronously flush.

Typically an IHtmlContent represents a small amount of content/text at this stage, because we flatten the list. So an IHtmlContent represents a single HTML attribute, or other small piece. However because the API is sync we can't do async while writing it.

rynowak · 2020-06-14T22:21:13Z

src/Http/WebUtilities/src/FileBufferingPipeWriter.cs

+    /// <summary>
+    /// A <see cref="Stream"/> that buffers content to be written to disk.
+    /// </summary>
+    public sealed class FileBufferingPipeWriter : PipeWriter, IAsyncDisposable


I'm not totally sure what the use case for buffering output to disk. I can't think of a place where we need that today. I know in Razor's use case we already have a pretty efficient in-memory representation of data, most of the complexity is in avoiding sync over async.

We've also gotten some really significant bad performance feedback about the use of disk for buffering I/O - so we'd like to avoid it when it's not absolutely required.

Revisiting this comment, it looks like we do this for XML serialization (which is synch). It's probably fine to leave the XML serialization alone and just let it use streams.

This is mainly were I've been stuck for a long time. My understanding is that views can not do chunking, so it requires the whole response to be buffered.
So it is not used only by the XML and Newtonsoft serializers, but also by the view executor:

aspnetcore/src/Mvc/Mvc.ViewFeatures/src/ViewComponentResultExecutor.cs

Line 152 in 9abf4bf

await using var bufferingStream = new FileBufferingWriteStream();

If this is instead only done to avoid sync over async, it will change eveything in this PR.
Or maybe I am missing something on how to avoid chunking.

rynowak · 2020-06-14T22:23:25Z

src/Http/WebUtilities/src/HttpResponsePipeWriter.cs

+
+            var length = _encoder.GetByteCount(value, false);
+            var buffer = _writer.GetSpan(length);
+            _encoder.GetBytes(value, buffer, false);


@GrabYourPitchforks - is there a more efficient way to do this? wondering about the pattern of GetByteCount immediately followed by GetBytes

rynowak · 2020-06-14T22:25:31Z

src/Http/WebUtilities/src/HttpResponsePipeWriter.cs

+                _disposed = true;
+                FlushEncoder();
+                // TOOD: flush
+                _writer.Complete();


this probably need to be surfaced in the public API (or not done) - since it seems similar to disposing the underlying stream. HttpResponseStreamWriter never disposes the stream

rynowak · 2020-06-14T22:28:45Z

src/Http/WebUtilities/src/HttpResponsePipeWriter.cs

+
+            Write(value.Span);
+            Write(NewLine);
+


It's a little bit of a red flag that the WriteAsync methods don't call PipeWriter.FlushAsync. This means that it's going to buffer until Flush is called by the caller even if you're using all of the async methods.

Would it not be switching to chunking if I flush?

Hi @rynowak , if I flush after every write it will cause sending lots of small chunks of a few bytes, as the rendering of a view involve a call to write for every element. What am I missing?

Currently it is buffered on disk so it is sent as a single chunk. I'm still not sure if that is a requirement (I believe it is given

aspnetcore/src/Http/WebUtilities/src/HttpResponseStreamWriter.cs

Lines 453 to 454 in 0889a62

// Note: our FlushInternal method does NOT flush the underlying stream. This would result in

// chunking.

) or if we can have a limited buffer just too avoid too much chunking. Who can shed some light on this?

Thank you!

rynowak · 2020-06-14T22:52:18Z

src/Mvc/Mvc.Abstractions/src/Formatters/OutputFormatterWriteContext.cs

@@ -20,7 +21,7 @@ public class OutputFormatterWriteContext : OutputFormatterCanWriteContext
        /// <param name="writerFactory">The delegate used to create a <see cref="TextWriter"/> for writing the response.</param>
        /// <param name="objectType">The <see cref="Type"/> of the object to write to the response.</param>
        /// <param name="object">The object to write to the response.</param>
-        public OutputFormatterWriteContext(HttpContext httpContext, Func<Stream, Encoding, TextWriter> writerFactory, Type objectType, object @object)
+        public OutputFormatterWriteContext(HttpContext httpContext, Func<PipeWriter, Encoding, TextWriter> writerFactory, Type objectType, object @object)


Unfortunately this isn't something we can change. We'd have to add a new property and a new constructor.

rynowak · 2020-06-14T22:53:42Z

src/Mvc/Mvc.Core/src/Infrastructure/IHttpResponseWriterFactory.cs

    /// </summary>
-    public interface IHttpResponseStreamWriterFactory
+    public interface IHttpResponseWriterFactory


Really the whole reason for this API was to abstract away the memory pool. Another option is to just don't when it comes to pipes. This change could be much more minimal if it didn't try to update this.

Agree. I started without but later I brought it back to have similar abstractions with the same level of indirection. I don't think there was a technical reason to use this, but I could be wrong as it was a while back. I'll try to remove it.

rynowak · 2020-06-14T22:55:47Z

src/Mvc/benchmarks/Microsoft.AspNetCore.Mvc.Performance/RuntimePerformanceBenchmarkBase.cs

@@ -49,7 +49,7 @@ private class NullLoggerFactory : ILoggerFactory, ILogger

        private class BenchmarkViewExecutor : ViewExecutor
        {
-            public BenchmarkViewExecutor(IOptions<MvcViewOptions> viewOptions, IHttpResponseStreamWriterFactory writerFactory, ICompositeViewEngine viewEngine, ITempDataDictionaryFactory tempDataFactory, DiagnosticListener diagnosticListener, IModelMetadataProvider modelMetadataProvider)
+            public BenchmarkViewExecutor(IOptions<MvcViewOptions> viewOptions, IHttpResponseWriterFactory writerFactory, ICompositeViewEngine viewEngine, ITempDataDictionaryFactory tempDataFactory, DiagnosticListener diagnosticListener, IModelMetadataProvider modelMetadataProvider)


It would be nice to see some changes that avoid PagedCharBufferTextWriter as well as what the impact is on perf 👍

davidfowl · 2020-06-14T23:52:35Z

src/Http/WebUtilities/src/FileBufferingPipeWriter.cs

+            _memoryThreshold = memoryThreshold;
+            _bufferLimit = bufferLimit;
+            _tempFileDirectoryAccessor = tempFileDirectoryAccessor ?? AspNetCoreTempDirectory.TempDirectoryFactory;
+            PagedByteBuffer = new PagedByteBuffer(ArrayPool<byte>.Shared);


I'm not sure we should be using this but now I'm convinced we should expose something in Pipelines for this.

Well this is not actually used here, I just brought it from the other implementation as I thought I would have needed it later to avoid chunking. Based on the outcome of that conversation, this can hopefully be avoided as otherwise it would not be possible to get big performance benefits.

Hey @davidfowl , any progress on exposing something in Pipelines to support this?

alefranz · 2020-07-19T16:12:31Z

Hey @davidfowl , is there any way with Pipelines to avoid excessive chunking? or should I keep track of how many bytes I have written to avoid calling FlushAsync to often?

src/Http/WebUtilities/src/HttpResponsePipeWriter.cs

alefranz · 2020-12-02T14:46:41Z

Does it make sense for me to resume the work on this to see if it could be included in 6.0?
Is there any plan to add the ability to control chunking in Pipeline?

javiercn · 2020-12-03T11:54:53Z

Does it make sense for me to resume the work on this to see if it could be included in 6.0?
Is there any plan to add the ability to control chunking in Pipeline?

We'll need to look at this change a bit in more depth, but I'm supportive of it. My concern is that the current area is very highly fine-tuned and I want to make sure we don't regress performance for important scenarios.

I think I would be more confortable working on it early in 6.0 than at the end, so I think this is a good time to start.

mkArtakMSFT · 2022-01-16T18:34:01Z

Hi. Looks like this PR seen no activity for a long time. What is the latest status @javiercn ? Do we plan to take this or should we close it?

pranavkm · 2022-01-21T15:03:05Z

Sorry @alefranz we dropped the ball on this. Unfortunately there are a number of merge conflicts here and it's unlikely we'll be able to spend time reviewing the changes for correctness for the first half of .NET 7. I'm going to close this for, we can engage with you if we end up deciding to do work here.

rynowak requested a review from pranavkm June 14, 2020 22:18

rynowak reviewed Jun 14, 2020

View reviewed changes

davidfowl reviewed Jun 14, 2020

View reviewed changes

mkArtakMSFT added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 15, 2020

mkArtakMSFT assigned pranavkm and javiercn and unassigned pranavkm Jun 17, 2020

mkArtakMSFT added the community-contribution Indicates that the PR has been added by a community member label Jul 20, 2020

alefranz force-pushed the HttpResponsePipeWriter branch 2 times, most recently from cceb2bd to 4b74562 Compare August 1, 2020 13:18

davidfowl reviewed Aug 1, 2020

View reviewed changes

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

alefranz mentioned this pull request Aug 2, 2020

Extensions.Logging: JsonConsoleFormatter serializes scope and state properties using native json type dotnet/runtime#40067

Merged

alefranz force-pushed the HttpResponsePipeWriter branch from f2d489a to e797602 Compare August 5, 2020 07:57

javiercn added the 6.0-candidate label Aug 19, 2020

alefranz added 4 commits December 3, 2020 14:27

Spiked HttpResponsePipeWriter

b245eb8

Addressed feedback and refactored

0e21d28

Removed breaking changes

0049b66

Public API

8b913b8

alefranz force-pushed the HttpResponsePipeWriter branch from e797602 to 8b913b8 Compare December 3, 2020 15:30

alefranz added 2 commits December 3, 2020 15:33

Simplified single char write

fb29029

Public API

fcf7004

Base automatically changed from master to main January 22, 2021 01:32

mkArtakMSFT removed the 6.0-candidate label Feb 24, 2021

pranavkm closed this Jan 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spiked HttpResponsePipeWriter #22916

Spiked HttpResponsePipeWriter #22916

alefranz commented Jun 13, 2020

rynowak commented Jun 14, 2020

rynowak Jun 14, 2020

rynowak Jun 14, 2020

alefranz Jun 15, 2020

rynowak Jun 14, 2020

rynowak Jun 14, 2020

rynowak Jun 14, 2020

alefranz Jun 15, 2020

alefranz Jul 18, 2020

rynowak Jun 14, 2020

rynowak Jun 14, 2020

alefranz Jun 15, 2020

rynowak Jun 14, 2020

davidfowl Jun 14, 2020

alefranz Jun 15, 2020 •

edited

Loading

alefranz Dec 3, 2020

alefranz commented Jul 19, 2020

alefranz commented Dec 2, 2020

javiercn commented Dec 3, 2020

mkArtakMSFT commented Jan 16, 2022

pranavkm commented Jan 21, 2022

	// Note: our FlushInternal method does NOT flush the underlying stream. This would result in
	// chunking.

Spiked HttpResponsePipeWriter #22916

Spiked HttpResponsePipeWriter #22916

Conversation

alefranz commented Jun 13, 2020

rynowak commented Jun 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alefranz Jun 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alefranz commented Jul 19, 2020

alefranz commented Dec 2, 2020

javiercn commented Dec 3, 2020

mkArtakMSFT commented Jan 16, 2022

pranavkm commented Jan 21, 2022

alefranz Jun 15, 2020 •

edited

Loading