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.

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

HttpClient creates Quic threads even if HTTP/3 is not in use #74629

Closed
wfurt opened this issue Aug 26, 2022 · 21 comments · Fixed by #74749, #75163 or #75441
Closed

HttpClient creates Quic threads even if HTTP/3 is not in use #74629

wfurt opened this issue Aug 26, 2022 · 21 comments · Fixed by #74749, #75163 or #75441
Assignees
Labels
Milestone

Comments

@wfurt
Copy link
Member

wfurt commented Aug 26, 2022

  1. Create simple console app with HTTP and run simple http1.1 request on Windows 11 or Server 22.
  2. look at application threads.

image

Application crates 2x threads per core even if HTTP/3 and Quic are not in used. This was originally reported on legacy system with 54 cores. That will create 100+ threads ... for nothing.

We should figure out how to make Quic.IsSupported cheap and defer thread creation until needed.
This will impact legacy app that has no clue tor intention to use Quic or HTTP/3.

@wfurt wfurt added tenet-compatibility Incompatibility with previous versions or .NET Framework area-System.Net.Quic labels Aug 26, 2022
@ghost
Copy link

ghost commented Aug 26, 2022

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

Issue Details
  1. Create simple console app with HTTP and run simple http1.1 request on Windows 11 or Server 22.
  2. look at application threads.

image

Application crates 2x threads per core even if HTTP/3 and Quic are not in used. This was originally reported on legacy system with 54 cores. That will create 100+ threads ... for nothing.

We should figure out how to make Quic.IsSupported cheap and defer thread creation until needed.
This will impact legacy app that has no clue tor intention to use Quic or HTTP/3.

Author: wfurt
Assignees: -
Labels:

tenet-compatibility, area-System.Net.Quic

Milestone: -

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 26, 2022
@rzikm
Copy link
Member

rzikm commented Aug 26, 2022

I think we can simply try opening and closing the API registration (that should deallocate the internal MsQuic threadpool), and then lazily reopen it when we actually intend to use it.

@stephentoub stephentoub added this to the 7.0.0 milestone Aug 28, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 28, 2022
@ghost
Copy link

ghost commented Aug 28, 2022

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

Issue Details
  1. Create simple console app with HTTP and run simple http1.1 request on Windows 11 or Server 22.
  2. look at application threads.

image

Application crates 2x threads per core even if HTTP/3 and Quic are not in used. This was originally reported on legacy system with 54 cores. That will create 100+ threads ... for nothing.

We should figure out how to make Quic.IsSupported cheap and defer thread creation until needed.
This will impact legacy app that has no clue tor intention to use Quic or HTTP/3.

Author: wfurt
Assignees: -
Labels:

area-System.Net.Http, tenet-compatibility, area-System.Net.Quic

Milestone: 7.0.0

@rzikm rzikm self-assigned this Aug 29, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 29, 2022
@karelz
Copy link
Member

karelz commented Aug 30, 2022

Triage: This contributes to 1MB per core Working set regression on Windows 2022/Win11+ and on Linux when msquic is present.
We should fix it in 7.0.

@karelz karelz added tenet-performance Performance related issue bug and removed tenet-compatibility Incompatibility with previous versions or .NET Framework labels Aug 30, 2022
@rzikm
Copy link
Member

rzikm commented Aug 31, 2022

I talked with Nick in MsQuic discord about this, MsQuicOpenVersion will allocate threads for handling UDP sockets, this is being made lazy by microsoft/msquic#3034, and another set of threads is allocated by RegistrationOpen.

One direction they consider moving is allowing applications (in our case, .NET Runtime) completely control the threads/scheduling, but that is distant future.

Closing MsQuic via MsQuicClose after checking for support and later opening it again lazily when needed is currently the best option.

@wfurt
Copy link
Member Author

wfurt commented Aug 31, 2022

Is it worth of picking up that change? It seems like we can suppress the ongoing thread cost but we would still create and destroy the threads when checking the version...

@rzikm
Copy link
Member

rzikm commented Aug 31, 2022

That MsQuic PR is making API changes, which would prevent it from being ported to the release branch.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 1, 2022
@rzikm
Copy link
Member

rzikm commented Sep 1, 2022

Reopening for backport

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 12, 2022
@karelz
Copy link
Member

karelz commented Sep 12, 2022

Based on discussion on the PR: #75330 (comment) ... we believe some fix should happen in 7.0 - either in RC2/GA or in servicing.

@karelz karelz modified the milestones: 8.0.0, 7.0.0 Sep 12, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 13, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 13, 2022
@karelz
Copy link
Member

karelz commented Sep 13, 2022

Reopening for 7.0 backport (though we may need to wait for more validation in main first).

@karelz karelz reopened this Sep 13, 2022
@karelz
Copy link
Member

karelz commented Sep 13, 2022

Open questions:

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 23, 2022
@karelz
Copy link
Member

karelz commented Sep 23, 2022

Fixed in main (8.0) in PRs #75163 and #75441 and in 7.0 (GA) in PR #75521 and in 6.0.15 in PR #80785.

@karelz karelz closed this as completed Sep 23, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 23, 2022
@karelz
Copy link
Member

karelz commented Jan 20, 2023

We have customers impacted on 6.0 as well - only via Kestrel. Reopening for 6.0.x backport.

@karelz karelz reopened this Jan 20, 2023
@karelz karelz modified the milestones: 7.0.0, 6.0.x Jan 20, 2023
@karelz karelz assigned ManickaP and unassigned rzikm Jan 20, 2023
@ManickaP
Copy link
Member

ManickaP commented Feb 15, 2023

Closing as #80785 is merged

@karelz
Copy link
Member

karelz commented Feb 21, 2023

Fixed also in 6.0.15 in PR #80785 -- see #74629 (comment) for details.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.