Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[H/3] Wait for WritesClosed and Dispose as fire-and-forget #104035

Merged
merged 3 commits into from
Jul 2, 2024
Merged

[H/3] Wait for WritesClosed and Dispose as fire-and-forget #104035

merged 3 commits into from
Jul 2, 2024

Conversation

ManickaP
Copy link
Member

Alternative to #104032. Solves the perf problem for waiting Dispose (with sync-over-async) by waiting on QuicStream.WritesClosed at the end of content writing together with doing QuicStream disposal as fire-and-forget in case the writes are already closed (avoiding potential data-loss).

cc @liveans

Fixes #77139

@liveans
Copy link
Member

liveans commented Jun 26, 2024

Related to #102571, let's run this on http benchmarks.

@ManickaP
Copy link
Member Author

I just ran the gRPC benchmarks on this:

  • 9.0 - base line
| load                    |                                       |
| ----------------------- | ------------------------------------- |
| Max CPU Usage (%)       | 73                                    |
| Max Cores usage (%)     | 2,037                                 |
| Max Working Set (MB)    | 460                                   |
| Max Private Memory (MB) | 2,578                                 |
| Build Time (ms)         | 12,851                                |
| Start Time (ms)         | 100                                   |
| Published Size (KB)     | 86,443                                |
| Symbols Size (KB)       | 176                                   |
| .NET Core SDK Version   | 9.0.100-preview.7.24323.5             |
| ASP.NET Core Version    | 9.0.0-preview.6.24320.4+613c1e990b6b  |
| .NET Runtime Version    | 9.0.0-preview.6.24319.11+117cfccdd71a |
| Server GC enabled       | True                                  |
| Processor Count         | 28                                    |
| First Request (ms)      | 1,482.92                              |
| Requests/sec            | 966,263                               |
| Requests                | 19,634,472                            |
| Bad responses           | 0                                     |
| Mean latency (ms)       | 0.77                                  |
| Max latency (ms)        | 15,285.50                             |
  • this PR
| load                    |                                       |
| ----------------------- | ------------------------------------- |
| Max CPU Usage (%)       | 73                                    |
| Max Cores usage (%)     | 2,033                                 |
| Max Working Set (MB)    | 433                                   |
| Max Private Memory (MB) | 1,209                                 |
| Build Time (ms)         | 7,754                                 |
| Start Time (ms)         | 101                                   |
| Published Size (KB)     | 86,443                                |
| Symbols Size (KB)       | 176                                   |
| .NET Core SDK Version   | 9.0.100-preview.7.24323.5             |
| ASP.NET Core Version    | 9.0.0-preview.6.24320.4+613c1e990b6b  |
| .NET Runtime Version    | 9.0.0-preview.6.24319.11+117cfccdd71a |
| Server GC enabled       | True                                  |
| Processor Count         | 28                                    |
| First Request (ms)      | 1,597.35                              |
| Requests/sec            | 3,850,558                             |
| Requests                | 20,011,352                            |
| Bad responses           | 0                                     |
| Mean latency (ms)       | 0.41                                  |
| Max latency (ms)        | 5,059.72                              |

The base line 966,263 RPS is not as bad as reported in #77139 (which was meager ~300k RPS). But this PR with 3,850,558 RPS is at the same numbers as before the issue was reported ~3.8m RPS

liveans
liveans approved these changes Jun 26, 2024
View reviewed changes
Copy link
Member

@liveans liveans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@liveans
Copy link
Member

liveans commented Jun 26, 2024

/azp run runtime-libraries stress-http

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dotnet dotnet deleted a comment from azure-pipelines bot Jun 26, 2024
@dotnet dotnet deleted a comment from liveans Jun 26, 2024
This was referenced Jun 26, 2024
Open
Open
Closed
@ManickaP
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

MihaZupan
MihaZupan reviewed Jun 27, 2024
View reviewed changes
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
@@ -88,7 +88,14 @@ public void Dispose()
{
_disposed = true;
AbortStream();
_stream.Dispose();
if (_stream.WritesClosed.IsCompleted)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is _stream.WritesClosed.IsCompleted expected to be true most of the time for basic GET/POST requests (when we're not doing bidirectional streaming)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

@ManickaP
Copy link
Member Author

ManickaP commented Jul 1, 2024

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP
Copy link
Member Author

ManickaP commented Jul 1, 2024

/azp run runtime-libraries stress-http

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP ManickaP marked this pull request as ready for review July 1, 2024 08:34
@ManickaP ManickaP requested a review from a team July 1, 2024 08:34
rzikm
rzikm approved these changes Jul 1, 2024
View reviewed changes
Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is green.

MihaZupan
MihaZupan reviewed Jul 1, 2024
View reviewed changes
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs Outdated
if (endStream)
{
await _stream.WritesClosed.ConfigureAwait(false);
}

await _stream.FlushAsync(cancellationToken).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels odd that we're flushing after we've ensured that writes are complete. If the flush is a no-op can we delete it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll switch the order.

@ManickaP
Copy link
Member Author

ManickaP commented Jul 1, 2024

/azp run runtime-libraries stress-http

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP
Copy link
Member Author

ManickaP commented Jul 1, 2024

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Jul 1, 2024
Open
@ManickaP
Copy link
Member Author

ManickaP commented Jul 2, 2024

/azp run runtime-libraries stress-http

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ManickaP ManickaP merged commit 5b0a2e6 into dotnet:main Jul 2, 2024
83 of 94 checks passed
@ManickaP ManickaP deleted the h3-dispose-perf branch July 2, 2024 12:32
@MihaZupan MihaZupan mentioned this pull request Jul 3, 2024
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2024
@karelz karelz added this to the 9.0.0 milestone Sep 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MihaZupan MihaZupan MihaZupan left review comments

@rzikm rzikm rzikm approved these changes

@liveans liveans liveans approved these changes

Assignees

@ManickaP ManickaP

Labels
area-System.Net.Quic
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.

HTTP/3 connection or stream shutdown is slow
5 participants
@ManickaP @liveans @MihaZupan @rzikm @karelz