http.sys; add opt-in support for kernel-mode response buffering

[mgravell]

http.sys; add opt-in support for kernel-mode response buffering

Adds support in for buffered responses when using http.sys, to avoid performance problems in small-write/high-latency scenarios.

Description

As per linked issue, when using http.sys, in some scenarios, high volumes of small writes with high latency may cause significant performance impact, due to lack of a Pipe buffer in the http.sys implementation; as a simple workaround, here we optionally enable the http.sys support for response buffering as a global flag.

This is exposed by a new HttpSysOptions.EnableKernelResponseBuffering option.

This work is based on the very old (and not mergeable) work in #418, with some additional input - credit to @davidni

It is also planned to back-port this feature to net6/net7 (with the property internal - no change to public API), via an app-context switch "Microsoft.AspNetCore.Server.HttpSys.EnableKernelResponseBuffering"; this switch detection is retained in the default value to avoid issues for consumers when upgrading.

We have briefly discussed some new feature to allow per-request enable/disable, but have not implemented that at the current time. We can revisit that in the future.

Also considered: implementing a full Pipe implementation in the http.sys code with the pipe consumer writing to http.sys; attempting to minimise code impact until we have a definite demand for that.

Fixes #14455

Fixes #5852

Impact is hard to benchmark in a local network - latency is key; however, the change has been successfully validated internally via a private build. Ask me for details if you're curious. As a summary of the validation scenario - a large payload streaming operation that reads (proxies) an external request and mirrors that to response writes:

• without this change: takes over 3 minutes and eventually fails (not having transferred all the information)
• with this change: completes successfully in 8-10 seconds

[ghost]

Thank you for your API proposal. I'm removing the api-ready-for-review label. API Proposals should be submitted for review through Issues based on this template.

[ghost]

Thank you for your API proposal. I'm removing the api-ready-for-review label. API Proposals should be submitted for review through Issues based on this template.

[mgravell]

OK bot; have it your way; API change submitted separately

[davidni]

Happy to see this finally making it to supported releases, thanks @mgravell!

[mgravell]

@davidni thanks for your original work - the above is almost exactly that (it was completely impossible to merge the original PR, sorry - but I did try to at least acknowledge credit!) - for your interest, only minor tweak was to ensure that all writes use the flag (to avoid http.sys getting confused), and the app-context switch (which you can probably infer means "someone needed the feature, and can't use net8 yet; we can't change the pre-net8-API without a very good reason"); the main problem I had here was validating it!

[Tratcher]

Can you at least summarize the perf improvements observed?

[BrennanConroy]

Can you at least summarize the perf improvements observed?

And has this impacted other scenarios besides the high latency + small writes + large payload scenario?

[mgravell]

Can you at least summarize the perf improvements observed?

added in main post

[BrennanConroy]

From the docs https://learn.microsoft.com/windows/win32/api/http/nf-http-httpsendresponseentitybody

Applications using asynchronous I/O which may have more than one send outstanding at a time should not use this flag.

Does this mean the entire application can't have concurrent sends? Are we preventing overlap when this is set? Am I misunderstanding the docs?

[amcasey]

@mgravell What's left here?

[JamesNK]

Blocked on API review: #47777

[mgravell]

@BrennanConroy we already block (synchronously or asynchronously as appropriate) individual writes - concurrent parallel writes and overlapped (i.e. not awaiting a WriteAsync) is always going to be unsupported, regardless of API or server implementation. Which is a long way of saying "no change here".

[davidni]

Side-note for posterity -- when I played with HTTP.sys perf on Asp .NET Core back in the day, one of our experiments was to NOT enable HTTP.sys buffering, and instead to do overlapped writes, ensuring there were always at least N outstanding writes. This helped improve perf in our scenarios (proxying large responses from a destination service -- before YARP), but (surprisingly at the time) we were never able to achieve the same throughput that http.sys kernel buffering afforded.

[ghost]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

[davidni]

This is no longer blocked, is it? Per #47777, the proposed api change is reviewed and approved.

Sugg removing the blocked tag.

[mgravell]

(rebased as part of final validations; preparing to merge)

[ghost]

@mgravell, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!