Skip to content

Remove the double dispatch on Windows for IO #43449

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
Merged
merged 1 commit into from
Aug 25, 2022
Merged

Remove the double dispatch on Windows for IO #43449

merged 1 commit into from
Aug 25, 2022

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Aug 22, 2022 •
edited
Loading

  • The windows IO thread pool was replaced with one that dispatches
    to the thread pool. As a result, we remove the extra thread pool dispatch
    that we originally had for windows because continuations for IO on windows ran on
    the same thread that polls for IO.

See dotnet/runtime#64834 for the thread pool changes.

PS: I think we need to adjust this for the old non portable threadpool (there's a flag to go back to the old mode that we should probably respect here as well). This should be in .NET 7.

TBD: Perf tests.

@ghost ghost added the area-runtime label Aug 22, 2022
@adityamandaleeka
Copy link
Member

Are we aiming for RC1 for this?

@davidfowl
Copy link
Member Author

Yes.

@adityamandaleeka
Copy link
Member

OK, can you change the target branch to release/7.0-rc1?

@adityamandaleeka
Copy link
Member

PS: I think we need to adjust this for the old non portable threadpool (there's a flag to go back to the old mode that we should probably respect here as well).

Is there? 😄 dotnet/runtime#71719

@davidfowl davidfowl marked this pull request as ready for review August 22, 2022 19:15
@davidfowl
Copy link
Member Author

Oh snap its gone!

@davidfowl davidfowl changed the base branch from main to release/7.0-rc1 August 22, 2022 19:29
@davidfowl davidfowl changed the base branch from release/7.0-rc1 to main August 22, 2022 19:29
@davidfowl davidfowl closed this Aug 22, 2022
@davidfowl davidfowl reopened this Aug 22, 2022
halter73
halter73 approved these changes Aug 22, 2022
View reviewed changes
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

Assuming that this improves benchmarks. Or at least doesn't regress it.

@davidfowl
Remove the double dispatch on Windows for IO
0293cb9 
- The windows IO thread pool was replaced with one that dispatches
to the thread pool. As a result, we remove the extra thread pool dispatch
that we originally had for windows because continuations for IO on windows ran on
the same thread that polls for IO.
@davidfowl davidfowl force-pushed the davidfowl/remove-double-dispatch branch from 502bddf to 0293cb9 Compare August 22, 2022 20:26
@davidfowl davidfowl changed the base branch from main to release/7.0-rc1 August 22, 2022 20:27
@dougbu
Copy link
Contributor

dougbu commented Aug 22, 2022

servicing-approved @adityamandaleeka or do we need a Steve email today❔

@adityamandaleeka
Copy link
Member

Waiting for benchmarks that @davidfowl is collecting.

@sebastienros
Copy link
Member

/benchmark plaintext aspnet-citrine-win kestrel

@pr-benchmarks
Copy link

pr-benchmarks bot commented Aug 24, 2022

Benchmark started for plaintext on aspnet-citrine-win with kestrel

@pr-benchmarks
Copy link

pr-benchmarks bot commented Aug 24, 2022

plaintext - aspnet-citrine-win

application plaintext.base plaintext.pr
CPU Usage (%) 79 84 +6.33%
Cores usage (%) 2,201 2,362 +7.31%
Working Set (MB) 106 104 -1.89%
Private Memory (MB) 135 132 -2.22%
Build Time (ms) 1,238 1,226 -0.97%
Start Time (ms) 337 342 +1.48%
Published Size (KB) 98,708 98,708 0.00%
.NET Core SDK Version 7.0.100-rc.2.22423.10 7.0.100-rc.2.22423.10
load plaintext.base plaintext.pr
CPU Usage (%) 85 87 +2.35%
Cores usage (%) 2,378 2,448 +2.94%
Working Set (MB) 38 38 0.00%
Private Memory (MB) 370 370 0.00%
Start Time (ms) 0 0
First Request (ms) 75 75 0.00%
Requests/sec 9,998,426 10,526,303 +5.28%
Requests 150,953,451 158,946,688 +5.30%
Mean latency (ms) 40.42 52.73 +30.46%
Max latency (ms) 721.58 1,440.00 +99.56%
Bad responses 0 0
Socket errors 0 0
Read throughput (MB/s) 1,198.08 1,269.76 +5.98%
Latency 50th (ms) 0.92 0.85 -7.61%
Latency 75th (ms) 41.00 39.42 -3.85%
Latency 90th (ms) 139.46 176.15 +26.31%
Latency 99th (ms) 397.92 587.72 +47.70%

@davidfowl
Copy link
Member Author

Oh good its faster 😄

@davidfowl
Copy link
Member Author

/benchmark json aspnet-citrine-win kestrel

@adityamandaleeka
Copy link
Member

adityamandaleeka commented Aug 24, 2022 •
edited
Loading

Is that 5% RPS real?

Edit: cool, didn't see your comment when I posted.

@pr-benchmarks
Copy link

pr-benchmarks bot commented Aug 24, 2022

Benchmark started for json on aspnet-citrine-win with kestrel

@pr-benchmarks
Copy link

pr-benchmarks bot commented Aug 24, 2022

json - aspnet-citrine-win

application json.base json.pr
CPU Usage (%) 80 78 -2.50%
Cores usage (%) 2,239 2,183 -2.50%
Working Set (MB) 74 77 +4.05%
Private Memory (MB) 102 105 +2.94%
Build Time (ms) 5,122 1,613 -68.51%
Start Time (ms) 341 347 +1.76%
Published Size (KB) 98,708 98,708 0.00%
.NET Core SDK Version 7.0.100-rc.2.22423.10 7.0.100-rc.2.22423.10
load json.base json.pr
CPU Usage (%) 63 76 +20.63%
Cores usage (%) 1,771 2,119 +19.65%
Working Set (MB) 38 38 0.00%
Private Memory (MB) 363 363 0.00%
Start Time (ms) 0 0
First Request (ms) 72 71 -1.39%
Requests/sec 914,268 1,109,706 +21.38%
Requests 13,804,615 16,755,859 +21.38%
Mean latency (ms) 0.66 1.02 +54.54%
Max latency (ms) 34.87 73.56 +110.95%
Bad responses 0 0
Socket errors 0 0
Read throughput (MB/s) 127.30 154.51 +21.37%
Latency 50th (ms) 0.42 0.30 -26.92%
Latency 75th (ms) 0.59 0.81 +37.69%
Latency 90th (ms) 1.51 2.79 +84.77%
Latency 99th (ms) 2.70 9.03 +234.44%

@davidfowl
Copy link
Member Author

/benchmark json plaintext aspnet-citrine-win kestrel

@davidfowl
Copy link
Member Author

/benchmark json, plaintext aspnet-citrine-win kestrel

@davidfowl
Copy link
Member Author

/benchmark json,plaintext aspnet-citrine-win kestrel

@pr-benchmarks
Copy link

pr-benchmarks bot commented Aug 24, 2022

Benchmark started for json, plaintext on aspnet-citrine-win with kestrel

@pr-benchmarks
Copy link

pr-benchmarks bot commented Aug 24, 2022

json - aspnet-citrine-win

application json.base json.pr
CPU Usage (%) 80 81 +1.25%
Cores usage (%) 2,237 2,255 +0.80%
Working Set (MB) 75 75 0.00%
Private Memory (MB) 102 103 +0.98%
Build Time (ms) 5,125 1,300 -74.63%
Start Time (ms) 335 336 +0.30%
Published Size (KB) 98,708 98,708 0.00%
.NET Core SDK Version 7.0.100-rc.2.22423.10 7.0.100-rc.2.22423.10
load json.base json.pr
CPU Usage (%) 64 75 +17.19%
Cores usage (%) 1,784 2,112 +18.39%
Working Set (MB) 38 38 0.00%
Private Memory (MB) 363 363 0.00%
Start Time (ms) 0 0
First Request (ms) 77 70 -9.09%
Requests/sec 917,094 1,101,743 +20.13%
Requests 13,847,530 16,636,002 +20.14%
Mean latency (ms) 0.67 1.09 +63.88%
Max latency (ms) 66.32 46.59 -29.75%
Bad responses 0 0
Socket errors 0 0
Read throughput (MB/s) 127.69 153.40 +20.13%
Latency 50th (ms) 0.42 0.31 -26.97%
Latency 75th (ms) 0.56 0.84 +49.46%
Latency 90th (ms) 1.53 2.96 +93.46%
Latency 99th (ms) 2.79 10.21 +265.95%

plaintext - aspnet-citrine-win

application plaintext.base plaintext.pr
CPU Usage (%) 71 74 +4.23%
Cores usage (%) 1,991 2,066 +3.77%
Working Set (MB) 104 105 +0.96%
Private Memory (MB) 132 132 0.00%
Build Time (ms) 1,637 1,255 -23.34%
Start Time (ms) 341 332 -2.64%
Published Size (KB) 98,708 98,708 0.00%
.NET Core SDK Version 7.0.100-rc.2.22423.10 7.0.100-rc.2.22423.10
load plaintext.base plaintext.pr
CPU Usage (%) 86 86 0.00%
Cores usage (%) 2,395 2,415 +0.84%
Working Set (MB) 38 38 0.00%
Private Memory (MB) 370 370 0.00%
Start Time (ms) 0 0
First Request (ms) 70 76 +8.57%
Requests/sec 9,991,470 10,397,715 +4.07%
Requests 150,865,147 157,006,960 +4.07%
Mean latency (ms) 46.22 31.34 -32.19%
Max latency (ms) 955.29 690.80 -27.69%
Bad responses 0 0
Socket errors 0 0
Read throughput (MB/s) 1,198.08 1,249.28 +4.27%
Latency 50th (ms) 0.92 0.85 -7.61%
Latency 75th (ms) 50.04 26.86 -46.32%
Latency 90th (ms) 162.79 114.13 -29.89%
Latency 99th (ms) 425.44 315.27 -25.90%

@pr-benchmarks
Copy link

pr-benchmarks bot commented Aug 24, 2022

Crank - Pull Request Bot

/benchmark <benchmarks[,...]> <profiles[,...]> <components,[...]>

Benchmarks:

  • plaintext: TechEmpower Plaintext Scenario - ASP.NET Platform implementation
  • json: TechEmpower JSON Scenario - ASP.NET Platform implementation
  • fortunes: TechEmpower Fortunes Scenario - ASP.NET Platform implementation
  • yarp: YARP - http-http with 10 bytes
  • mvcjsoninput2k: Sends 2Kb Json Body to an MVC controller

Profiles:

  • aspnet-perf-lin: INTEL/Linux 12 Cores
  • aspnet-perf-win: INTEL/Windows 12 Cores
  • aspnet-citrine-lin: INTEL/Linux 28 Cores
  • aspnet-citrine-win: INTEL/Windows 28 Cores
  • aspnet-citrine-arm: ARM/Linux 32 Cores
  • aspnet-citrine-amd: AMD/Linux 48 Cores

Components:

  • kestrel
  • mvc

@davidfowl
Copy link
Member Author

@adityamandaleeka approve?

@JamesNK
Copy link
Member

JamesNK commented Aug 24, 2022

The req/s increase is nice. Did you expect a big increase?

@davidfowl
Copy link
Member Author

davidfowl commented Aug 24, 2022 •
edited
Loading

The req/s increase is nice. Did you expect a big increase?

Yes. We did the same for linux a while back.

@dougbu
Copy link
Contributor

dougbu commented Aug 24, 2022

Approved for RC1 @adityamandaleeka or @Pilchie (if you're back)❔

@adityamandaleeka
Copy link
Member

Approved, this is a good win and getting validation over 2 RCs instead of 1 seems worthwhile.

@adityamandaleeka adityamandaleeka merged commit d2c7b65 into release/7.0-rc1 Aug 25, 2022
@adityamandaleeka adityamandaleeka deleted the davidfowl/remove-double-dispatch branch August 25, 2022 00:24
@davidfowl davidfowl added the Perf label Aug 26, 2022
@davidfowl davidfowl added this to the 7.0-rc1 milestone Aug 26, 2022
@sebastienros sebastienros mentioned this pull request Aug 31, 2022
Closed
@adityamandaleeka adityamandaleeka added the blog-candidate Consider mentioning this in the release blog post label Sep 6, 2022
@ghost
Copy link

ghost commented Sep 6, 2022

@davidfowl, 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!

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
halter73 pushed a commit to halter73/AspNetCore that referenced this pull request Nov 20, 2024
@davidfowl @halter73
Remove the double dispatch on Windows for IO (dotnet#43449)
ae97305 
The windows IO thread pool was replaced with one that dispatches to the thread pool. As a result, we remove the extra thread pool dispatch that we originally had for windows because continuations for IO on windows ran on
the same thread that polls for IO.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halter73 halter73 halter73 approved these changes

@JamesNK JamesNK JamesNK approved these changes

@Tratcher Tratcher Tratcher approved these changes

@adityamandaleeka adityamandaleeka Awaiting requested review from adityamandaleeka

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy

@dougbu dougbu Awaiting requested review from dougbu

@wtgodbe wtgodbe Awaiting requested review from wtgodbe

@captainsafia captainsafia Awaiting requested review from captainsafia

@mkArtakMSFT mkArtakMSFT Awaiting requested review from mkArtakMSFT

@Pilchie Pilchie Awaiting requested review from Pilchie

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions blog-candidate Consider mentioning this in the release blog post Perf
Projects
None yet
Milestone
7.0-rc1
Development

Successfully merging this pull request may close these issues.
8 participants
@davidfowl @adityamandaleeka @dougbu @sebastienros @JamesNK @halter73 @Tratcher @amcasey