Implement dynamic HTTP/2 window scaling #54755

antonfirsov · 2021-06-25T18:02:05Z

Fixes #43086 by introducing automatic scaling of the HTTP/2 stream receive window based on measuring RTT of PING frames.

The algorithm has been tested and benchmarked against various bandwidth-RTT combos. The HTTP/2 download speed still does not match HTTP/1, but this is also true for a large static window, we need to do a separate investigation on this.

The PR introduces 3 tuning options as environment variables/AppContext switches:

DOTNET_SYSTEM_NET_HTTP_SOCKETSHTTPHANDLER_HTTP2FLOWCONTROL_DISABLEDYNAMICWINDOWSIZING to disable the dynamic window alogithm and the RTT PING
DOTNET_SYSTEM_NET_HTTP_SOCKETSHTTPHANDLER_FLOWCONTROL_MAXSTREAMWINDOWSIZE to control the maximum stream window size
DOTNET_SYSTEM_NET_HTTP_SOCKETSHTTPHANDLER_FLOWCONTROL_STREAMWINDOWSCALETHRESHOLDMULTIPLIER to enable fine-tuning the balance between optimal receive window VS download speed

Resolves #53372.

I'm still looking at a failing test (ResponseStreamFrames_DataAfterHeadersAndContinuationWithoutEndHeaders_ConnectionError) and I want to re-validate in an environment with actual physical (non-simulated) delay, but generally the PR is ready for review.

@geoffkizer I addressed the naming concerns you raised in #52862, hope it looks better now.

/cc @ManickaP @halter73 @JamesNK @brporter

dotnet-issue-labeler · 2021-06-25T18:02:10Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-06-25T18:02:11Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #43086 by introducing automatic scaling of the HTTP/2 stream receive window based on measuring RTT of PING frames.

The algorithm has been tested and benchmarked against various bandwidth-RTT combos. The HTTP/2 download speed still does not match HTTP/1, but this is also true for a large static window, we need to do a separate investigation on this.

The PR introduces 3 tuning options as environment variables/AppContext switches:

DOTNET_SYSTEM_NET_HTTP_SOCKETSHTTPHANDLER_HTTP2FLOWCONTROL_DISABLEDYNAMICWINDOWSIZING to disable the dynamic window alogithm and the RTT PING
DOTNET_SYSTEM_NET_HTTP_SOCKETSHTTPHANDLER_FLOWCONTROL_MAXSTREAMWINDOWSIZE to control the maximum stream window size
DOTNET_SYSTEM_NET_HTTP_SOCKETSHTTPHANDLER_FLOWCONTROL_STREAMWINDOWSCALETHRESHOLDMULTIPLIER to enable fine-tuning the balance between optimal receive window VS download speed

Resolves #53372.

I'm still looking at a failing test (ResponseStreamFrames_DataAfterHeadersAndContinuationWithoutEndHeaders_ConnectionError) and I want to re-validate in an environment with actual physical (non-simulated) delay, but generally the PR is ready for review.

/cc @geoffkizer @ManickaP @halter73 @JamesNK @brporter

Author:	antonfirsov
Assignees:	-
Labels:	`area-System.Net.Http`, `new-api-needs-documentation`
Milestone:	-

# Conflicts: # src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs

geoffkizer · 2021-06-28T19:10:50Z

I hesitate a little bit to expose DOTNET_SYSTEM_NET_HTTP_SOCKETSHTTPHANDLER_FLOWCONTROL_MAXSTREAMWINDOWSIZE and DOTNET_SYSTEM_NET_HTTP_SOCKETSHTTPHANDLER_FLOWCONTROL_STREAMWINDOWSCALETHRESHOLDMULTIPLIER because once we expose them, someone will depend on them and it will be nearly impossible to remove these.

How important do we think these really are?

antonfirsov · 2021-06-28T19:44:11Z

someone will depend on them and it will be nearly impossible to remove these.

In case they will become obsolete, they will be just NOP-s, so I don't think this is a problem.

How important do we think these really are?

I can't judge if there are customers who are really interested tweaking them, although in our internal discussions we assumed that there will be some. TBH, if someone really wants to tweak this stuff for faster download, they should just bump the initial window size. The question is if there's anyone interested overriding them for lower memory footprint.

geoffkizer

Posting the feedback I have so far, more to come later.

src/libraries/Common/src/System/Net/Http/HttpHandlerDefaults.cs

src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs

src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionSettings.cs

geoffkizer · 2021-06-28T19:48:02Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionSettings.cs

-                // First check for the AppContext switch, giving it priority over the environment variable.
-                if (AppContext.TryGetSwitch(Http3DraftSupportAppCtxSettingName, out bool allowHttp3))
+                // Disallow negative values:
+                if (value < 0)


0 is valid?

There is no practical functional difference between 0 and double.Epsilon, these are mathematically valid values which will practically disable the scaling. Should we bother setting an arbitrary minimum?

(The question only matters if we want to keep the knob.)

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs

src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs

ManickaP · 2021-06-29T14:55:00Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionSettings.cs

+        internal bool _disableDynamicHttp2WindowSizing = DisableDynamicHttp2WindowSizing;
+        internal int _maxHttp2StreamWindowSize = MaxHttp2StreamWindowSize;
+        internal double _http2StreamWindowScaleThresholdMultiplier = Http2StreamWindowScaleThresholdMultiplier;
+        internal int _initialHttp2StreamWindowSize = Http2Connection.DefaultInitialWindowSize;


Shouldn't Http2Connection.DefaultInitialWindowSize be in HttpHandlerDefaults then?

It should, yeah.

I also wonder whether 64K is still the right value for this. Should we make this larger?

Actually it wasn't optimal in any of my runs. Data from 0ms RTT / 5Gbps (loopback communication) benchmarks suggests it is worth to set it to 256K, however the window will scale faster to larger than optimal maximum sizes in all cases then.

I'm a bit hesitant to change the defaults without having more data. Maybe we should invest a couple of days to find the right defaults for the August preview?

ManickaP · 2021-06-29T15:00:10Z

...libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs

+
+            private int _deliveredBytes;
+            private int _streamWindowSize;
+            private long _lastWindowUpdate;


You could potentially use ValueStopwatch here. It remembers _startTimestamp and has public TimeSpan GetElapsedTime(). I haven't used it extensively but @MihaZupan did for telemetry/counters.

I stole the the unit conversion logic from ValueStopWatch because it seemed to be too heavy 😆 We don't need _startTimestamp here, we only need differences compared to the last timestamp.

Well, the _startTimestamp would be your _lastWindowUpdate...

A bit hesitant to change & rebenchmark at this point, may try on Monday.

What I mean is that:

TimeSpan dt = _stopwatch.GetElapsedTime(); _stopwatch = ValueStopwatch.StartNew();

will call Stopwatch.GetTimestamp() (OS call) twice. Although this wouldn't be a noticeable perf issue, I was actually afraid that someone will complain about this in the code review out of perfectionism, so went with manual timestamping instead of ValueStopwatch 😄 Don't want to change this now, want to fix my test and the remaining nits, and get this merged ASAP.

...libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs

...libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs

Co-authored-by: Geoff Kizer <geoffrek@microsoft.com>

…tend the window

ManickaP · 2021-07-02T13:44:22Z

...libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs

+
+            private int _deliveredBytes;
+            private int _streamWindowSize;
+            private long _lastWindowUpdate;


Well, the _startTimestamp would be your _lastWindowUpdate...

src/libraries/Common/src/System/Net/Http/HttpHandlerDefaults.cs

antonfirsov · 2021-07-02T16:12:38Z

One of my new tests failed on the CI because of timing issues. I will try to harden it on Monday, if it's not possible I will delete it.

Unfortunately there is no way to test this feature without extensive use of Task.Delay.

src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs

…topsScaling

antonfirsov · 2021-07-06T00:59:15Z

All CI failures are unrelated. I had to remove the test LowBandwidthDelayProduct_ClientStreamReceiveWindowStopsScaling, looks like it's not possible to reliably test this particular characteristic.

@geoffkizer @ManickaP if there's nothing critical left, I'll merge this within ~36 hours.

ManickaP

LGTM, thanks for all the research and benchmarking to do this 🚀

JamesNK · 2021-07-08T00:07:48Z

Nice!

Is there a before and after benchmark of this? I'd like to talk about it as a performance improvement when using gRPC and it would be good to have numbers to back that up.

e.g. before it took X seconds to download 100 megabytes, now it takes Y seconds.

antonfirsov · 2021-07-08T00:30:34Z

@JamesNK it's really a function of network delay and bandwidth. If the peers communicate over localhost, the difference is negligible. If there is a delay of several 10s of milliseconds with a relatively high bandwidth, it can be an order of magnitude or even more. I will share my spreadsheet with you tomorrow. Might make sense to make a GH-friendly report out of it, but I just started my vacation, may do it when I'm back :)

JamesNK · 2021-07-08T00:53:10Z

I get that it is variable. One or two examples to give people an idea of the kind of performance improvement it can get is what I'm looking for. Devs love this kind of stuff.

No rush.

Completely redesign tests for HTTP/2 KeepAlive PING, so they: - Work well with RTT pings introduced in Implement dynamic HTTP/2 window scaling #54755 - Run sequentially, reducing the chance of failing because of timing issues caused by parallel workloads - Are better organized: multiple test cases for different scenarios, instead of one theory with complex branches on parameters Instead of reading / reacting to frames inline, there is a separate Task for processing incoming frames, responding to PING immediately and pushing other frames to a Channel<Frame>. Fixes #41929

Implement dynamic HTTP2 window scaling

1d41853

dotnet-issue-labeler bot added area-System.Net.Http new-api-needs-documentation labels Jun 25, 2021

antonfirsov added 4 commits June 25, 2021 20:06

Merge branch 'main' into http/dynamic-window-02

4067443

# Conflicts: # src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs

actually fix the conflict with dotnet#54437

a3800d3

add comment on PING payloads

9f50764

delete StringBuilderOutput

3c7c474

antonfirsov commented Jun 25, 2021

View reviewed changes

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs Outdated Show resolved Hide resolved

antonfirsov added 2 commits June 25, 2021 20:25

DisableDynamicWindowSizing: fix name

6eb66e9

make RttEstimator.MinRtt thread-safe

4c7f3ac

This comment has been minimized.

Sign in to view

karelz mentioned this pull request Jun 28, 2021

[HTTP/3] Test failure System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClientHandler_Cancellation_Test_Http3_Mock.ConnectTimeout_TimesOutSSLAuth_Throws #54806

Closed

antonfirsov added 4 commits June 28, 2021 14:38

simplify RuntimeSettingParserTest code

473139d

respond to PING while reading CONTINUATION frames in ProcessHeadersFrame

f2ff4ee

fix test

2bf13e0

add PingBeforeContinuationFrame_Success

526d723

This comment has been minimized.

Sign in to view

antonfirsov commented Jun 28, 2021

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs Outdated Show resolved Hide resolved

geoffkizer suggested changes Jun 28, 2021

View reviewed changes

ManickaP reviewed Jun 29, 2021

View reviewed changes

geoffkizer reviewed Jun 29, 2021

View reviewed changes

...libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs Outdated Show resolved Hide resolved

geoffkizer reviewed Jun 29, 2021

View reviewed changes

...libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs Show resolved Hide resolved

geoffkizer reviewed Jun 29, 2021

View reviewed changes

...libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs Show resolved Hide resolved

geoffkizer reviewed Jun 29, 2021

View reviewed changes

...libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs Show resolved Hide resolved

antonfirsov and others added 3 commits July 2, 2021 11:06

Apply suggestions

95f9e0d

Co-authored-by: Geoff Kizer <geoffrek@microsoft.com>

separate _expectPingFrame and _transparentPingResponse functionality

aba1735

check for _streamWindowSize < MaxStreamWindowSize before trying to ex…

5cbf0e1

…tend the window

This comment has been minimized.

Sign in to view

ManickaP reviewed Jul 2, 2021

View reviewed changes

geoffkizer reviewed Jul 2, 2021

View reviewed changes

src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs Outdated Show resolved Hide resolved

antonfirsov added 3 commits July 5, 2021 17:55

nit

cd94dcf

move DefaultInitialHttp2StreamWindowSize

6954f61

harden LowBandwidthDelayProduct_ClientStreamReceiveWindowStopsScaling

6dbfa47

This comment has been minimized.

Sign in to view

delete unreliable LowBandwidthDelayProduct_ClientStreamReceiveWindowS…

3009773

…topsScaling

This comment has been minimized.

Sign in to view

ManickaP approved these changes Jul 6, 2021

View reviewed changes

antonfirsov merged commit c7ffa32 into dotnet:main Jul 8, 2021

steveisok mentioned this pull request Jul 8, 2021

[main] Update dependencies from dotnet/runtime dotnet/sdk#18801

Merged

captainsafia mentioned this pull request Jul 8, 2021

[main] Update dependencies from dotnet/efcore dotnet/runtime dotnet/aspnetcore#34033

Merged

karelz added this to the 6.0.0 milestone Jul 15, 2021

karelz mentioned this pull request Jul 20, 2021

Connect_DualMode_DnsConnect_RetrievedEndPoints_Success - SocketException: Transport endpoint is not connected #55709

Closed

karelz mentioned this pull request Jul 28, 2021

[WinHttpHandler] Long running test: ResponseHeadersRead_SynchronizationContextNotUsedByHandler on Win7/Win8 #54034

Closed

antonfirsov mentioned this pull request Aug 2, 2021

Redisign HTTP/2 KeepAlive PING tests #56736

Merged

ghost locked as resolved and limited conversation to collaborators Aug 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement dynamic HTTP/2 window scaling #54755

Implement dynamic HTTP/2 window scaling #54755

antonfirsov commented Jun 25, 2021 •

edited

Loading

dotnet-issue-labeler bot commented Jun 25, 2021

ghost commented Jun 25, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

geoffkizer commented Jun 28, 2021

antonfirsov commented Jun 28, 2021 •

edited

Loading

geoffkizer left a comment

geoffkizer Jun 28, 2021

antonfirsov Jun 29, 2021 •

edited

Loading

ManickaP Jun 29, 2021

geoffkizer Jun 29, 2021

antonfirsov Jun 29, 2021 •

edited

Loading

ManickaP Jun 29, 2021

antonfirsov Jun 29, 2021

ManickaP Jul 2, 2021

antonfirsov Jul 2, 2021

antonfirsov Jul 5, 2021

This comment has been minimized.

This comment has been minimized.

ManickaP Jul 2, 2021

antonfirsov commented Jul 2, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

antonfirsov commented Jul 6, 2021

ManickaP left a comment

JamesNK commented Jul 8, 2021

antonfirsov commented Jul 8, 2021 •

edited

Loading

JamesNK commented Jul 8, 2021 •

edited

Loading

Implement dynamic HTTP/2 window scaling #54755

Implement dynamic HTTP/2 window scaling #54755

Conversation

antonfirsov commented Jun 25, 2021 • edited Loading

dotnet-issue-labeler bot commented Jun 25, 2021

ghost commented Jun 25, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

geoffkizer commented Jun 28, 2021

antonfirsov commented Jun 28, 2021 • edited Loading

geoffkizer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Jun 29, 2021 • edited Loading

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

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

antonfirsov commented Jul 2, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

antonfirsov commented Jul 6, 2021

ManickaP left a comment

Choose a reason for hiding this comment

JamesNK commented Jul 8, 2021

antonfirsov commented Jul 8, 2021 • edited Loading

JamesNK commented Jul 8, 2021 • edited Loading

antonfirsov commented Jun 25, 2021 •

edited

Loading

antonfirsov commented Jun 28, 2021 •

edited

Loading

antonfirsov Jun 29, 2021 •

edited

Loading

antonfirsov Jun 29, 2021 •

edited

Loading

antonfirsov commented Jul 8, 2021 •

edited

Loading

JamesNK commented Jul 8, 2021 •

edited

Loading