Fix corrupted streaming SSR output

[SteveSandersonMS]

Fixes #50198

For details about the problem causing corrupted output, see #50198 (comment). As part of investigating, I also discovered that the MVC-style ViewBufferTextWriter solution performed very badly, reducing the number of requests per second by over half (compared with unbuffered output).

So, the fix here is to replace the buffering layer with a new implementation that is:

• Able to deal with writes and flushes occurring while another FlushAsync is still in progress (fixing the corruption)
• Much more focused and eliminates extra layers like IHtmlContent that we were not actually using (fixing the performance)

Benchmarks

Consider two cases:

• Small page: BlazorUnitedSample's Index page but with the <EditForm> commented out, so it's just rendering the layout with nothing on the page.
• Huge page: BlazorUnitedSample's FetchData page but with the number of weather forecasts increased to 5000 and the await Task.Delay removed.

Implementation	Small page (req/sec)	Huge page (req/sec)
Unbuffered	59095	fails
ViewBufferTextWriter (i.e., before this PR)	26447	610, but corrupted
TextChunkListBuilder + pooling	57543	478
TextChunkListBuilder, no pooling (this PR)	56683	495
MemoryStream	51705	343
List<>	54385	212

• Unbuffered is before Support view buffer-based rendering in Blazor #49860, which as you can see cannot handle large output. However when it works, it should be something like an upper bound for the other cases.
• In Support view buffer-based rendering in Blazor #49860 we added ViewBufferTextWriter which no longer fails the response for large output, but introduces the corruption
• MemoryStream and List are both simplistic alternative buffering strategies to check whether it's worth a more sophisticated solution. They both produce correct output but struggle on perf with larger output sizes, most likely because they have to keep copying state each time the backing buffer expands.
• TextChunkListBuilder is the page-based solution from this PR
  • Initially I implemented the same pooling strategy used by ViewBufferTextWriter, i.e., gets pages from ArrayPool.Shared with a per-response pool on top of that to reduce contention.
  • Then I tried stripping out all the pooling. The net result was virtually no change to throughput (the numbers do fluctuate, and I took the numbers that happened on a particular predecided run to avoid cherry-picking), while greatly simplifying the code and avoiding the risk of correctness problems if code fails to dispose properly.
  • I guess that the costs of managing pool contention, and having to clear arrays before returning to the pool, roughly balance out with the GC cost from not pooling. Hence I prefer the simpler implementation, even though it will show up more clearly in GC profiling.

Note that after measuring the above, I also added a further optimization to TextChunkListBuilder and now just measured 64233 but didn't re-run the other cases so it's possible my laptop is just being faster right now.

E2E test

It's hard to repro #50198 in an E2E test because it's based on timing of network traffic. I found a way to reproduce the corruption by picking specific numbers that surface the problem when I run locally. It's possible that on another machine the E2E test would pass anyway even without this fix. However with this fix, the E2E test should always pass.

[SteveSandersonMS]

It's always assigned during the constructor anyway.

[javiercn]

Aren't we buffering twice? Once here, and once in the BufferedTextWriter?

[SteveSandersonMS]

Yes, that's a good point. I retained this usage of HttpResponseStreamWriter from the earlier implementation but it might be that we could simply omit it and have TextChunkListBuilder able to WriteAsync directly to Response.Body.

I think the benefit of HttpResponseStreamWriter is that it's making fewer calls to Encoding.UTF8.Encode, as it only encodes once each time its buffer fills up. But I don't really know what level of perf gain this gives, if any.

I'll benchmark it both ways and go with whatever appears to be more performant. That sound OK?

[javiercn]

I just saw it and I think we would remove one buffering layer, which is generally good.

[SteveSandersonMS]

OK, I looked into this and tried replacing HttpResponseStreamWriter with:

1. StreamWriter. This works but is marginally slower than HttpResponseStreamWriter. Perhaps the default buffer size in HttpResponseStreamWriter is just better tuned to these cases. They are doing mostly the same things.
2. No string encoding buffer at all and writing directly to the Stream. This was much more complicated, since it's necessary to then replicate a lot of what StreamWriter or HttpResponseStreamWriter do to encode the different types of things we might be writing. And doing it in a simplistic way resulted in much lower perf than the above techniques - I suspect the optimal way might turn out to be the same as copying the internals from StreamWriter or HttpResponseStreamWriter.

So I think we'll stick with HttpResponseStreamWriter as in practice it appears to give the best results, and is the most well-tested code path in this scenario. It was worth looking into though, so thanks for the suggestion!

[javiercn]

The solution looks reasonable, but I have a higher-level question.

Instead of dealing with this at the buffer level. Would it make sense to deal with this at the HtmlRenderer somehow?

Is the challenge here that since UpdateDisplayAsync is not awaited, the contents of the RenderTreeFrames might change while we are waiting to write the response and the rendering continues?

Is that the reason why we need to buffer in this way?

• Would it work if we buffered more like Blazor Server (Take a Snapshot) then continue rendering? And then simply "emit" the sequence of snapshots (which are the updates I believe).
  • Is my understanding correct in that this is more or less what's already happening?

[SteveSandersonMS]

Is my understanding correct in that this is more or less what's already happening?

It is more or less equivalent. The only difference is that the approach used here (TextChunkListBuilder) is building a representation that's more similar to the final HTML output, which is marginally advantageous since we already have the logic for doing that in .NET (i.e., that's what prerendering has always done), whereas we don't have or need any .NET implementation of SerializedRenderBatch->HTML.

Strictly speaking this is an internal implementation detail - we could change the buffering format in the future if we wanted - but this PR is the more minimal change vs what we've been testing so far in .NET 8, and I'm not aware we'd get any extra benefit from going through a SerializedRenderBatch representation.

[javiercn]

Strictly speaking this is an internal implementation detail - we could change the buffering format in the future if we wanted - but this PR is the more minimal change vs what we've been testing so far in .NET 8, and I'm not aware we'd get any extra benefit from going through a SerializedRenderBatch representation.

The current approach is fine I think, I was mostly asking because it's a bit hard to reason about the interaction at this level. I was not suggesting that we serialize the render batch either (exactly like we do on Blazor Server), but wanted to draw the parallel to see if I was getting what was going on correctly.

We might benefit in the future from pushing this directly into the HTML renderer, as it might simplify the interactions (it would look more like what happens in the remote renderer, again).