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

Unload MsQuic after checking for QUIC support to free resources #75163

Merged
merged 4 commits into from
Sep 8, 2022

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Sep 6, 2022

Fixes #74629

This PR unloads MsQuic library from the process after we check whether it is supported. This saves some resources when HTTP/3 is not used (e.g. the server does not advertise HTTP/3)

Test app:

using System.Net.Http;

HttpClient client = new HttpClient();

await client.GetAsync("https://www.microsoft.com");

System.Console.WriteLine("Done");
System.Console.ReadLine();

Results on Windows11

Diff Before After
Threads 62 23
PrivateMemorySize 13385728 12083200

Brings back PR #74749, reverted by PR #74984

@ghost
Copy link

ghost commented Sep 6, 2022

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

Issue Details

Brings back #74749, reverted by #74984

Author: wfurt
Assignees: -
Labels:

area-System.Net

Milestone: -

@ghost
Copy link

ghost commented Sep 6, 2022

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

Issue Details

Brings back #74749, reverted by #74984

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net, area-System.Net.Quic

Milestone: -

@wfurt wfurt requested a review from rzikm September 7, 2022 01:46
@wfurt wfurt marked this pull request as ready for review September 7, 2022 01:46
@wfurt
Copy link
Member Author

wfurt commented Sep 7, 2022

Debian 10 on ARM64 is OK
https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-75163-merge-ee93b0bfb1f4439da8/System.Net.Quic.Functional.Tests/1/console.99482cb3.log?helixlogtype=result

Console log: 'System.Net.Quic.Functional.Tests' from job ee93b0bf-b1f4-439d-a82a-e3bbd340118b (ubuntu.1804.armarch.open) using docker image mcr.microsoft.com/dotnet-buildtools/prereqs:debian-10-helix-arm64v8-20220906200500-06f234f on ddvsotx2l103


/root/helix/work/workitem/e /root/helix/work/workitem/e
  Discovering: System.Net.Quic.Functional.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Net.Quic.Functional.Tests (found 106 of 112 test cases)
  Starting:    System.Net.Quic.Functional.Tests (parallel test collections = on, max threads = 6)
    System.Net.Quic.Tests.MsQuicPlatformDetectionTests.UnsupportedPlatforms_ThrowsPlatformNotSupportedException [SKIP]
      Condition(s) not met: "IsQuicUnsupported"
  Finished:    System.Net.Quic.Functional.Tests
=== TEST EXECUTION SUMMARY ===
   System.Net.Quic.Functional.Tests  Total: 326, Errors: 0, Failed: 0, Skipped: 1, Time: 124.431s

@rzikm
Copy link
Member

rzikm commented Sep 7, 2022

Would you mind picking changes from #74985 here as well?

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!

@karelz karelz changed the title Revert "Revert "Unload MsQuic after checking for QUIC support to free resources. (#74749)"" Unload MsQuic after checking for QUIC support to free resources Sep 8, 2022
@karelz karelz added this to the 8.0.0 milestone Sep 8, 2022
@karelz
Copy link
Member

karelz commented Sep 8, 2022

@wfurt I assume your Debian 10 ARM64 validation above was with msquic 2.1.1 package. Can you please confirm?

@karelz
Copy link
Member

karelz commented Sep 8, 2022

Also, should we mark the PR as blocked until we finish updating msquic 2.1.1 properly? (incl. Windows source code snapshot)

@wfurt
Copy link
Member Author

wfurt commented Sep 8, 2022

no, that is still floating in. It has Docker image based on dotnet/msquic main e.g. has startup changes but it is not the official bits.

@wfurt wfurt merged commit e995620 into main Sep 8, 2022
@wfurt wfurt deleted the revert-74984-73871-revert branch September 8, 2022 16:48
@rzikm
Copy link
Member

rzikm commented Sep 9, 2022

/backport to release/7.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2022

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3020723954

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2022

@rzikm backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Revert "Revert "Unload MsQuic after checking for QUIC support to free resources. (#74749)" (#74984)"
Applying: update helix images
Using index info to reconstruct a base tree...
M	eng/pipelines/libraries/helix-queues-setup.yml
Falling back to patching base and 3-way merge...
Auto-merging eng/pipelines/libraries/helix-queues-setup.yml
CONFLICT (content): Merge conflict in eng/pipelines/libraries/helix-queues-setup.yml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 update helix images
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

rzikm added a commit to rzikm/dotnet-runtime that referenced this pull request Sep 9, 2022
…et#75163)

* Revert "Revert "Unload MsQuic after checking for QUIC support to free resources. (dotnet#74749)" (dotnet#74984)"

This reverts commit 953f524.

* update helix images

* update helix images

* Improve diagnostics when opening MsQuic

Co-authored-by: Radek Zikmund <radekzikmund@microsoft.com>
rzikm added a commit to rzikm/dotnet-runtime that referenced this pull request Sep 13, 2022
…et#75163)

* Revert "Revert "Unload MsQuic after checking for QUIC support to free resources. (dotnet#74749)" (dotnet#74984)"

This reverts commit 953f524.

* update helix images

* update helix images

* Improve diagnostics when opening MsQuic

Co-authored-by: Radek Zikmund <radekzikmund@microsoft.com>
carlossanlop pushed a commit that referenced this pull request Sep 23, 2022
…sources (#75163, #75441) (#75521)

* Unload MsQuic after checking for QUIC support to free resources (#75163)

* Revert "Revert "Unload MsQuic after checking for QUIC support to free resources. (#74749)" (#74984)"

This reverts commit 953f524.

* update helix images

* update helix images

* Improve diagnostics when opening MsQuic

Co-authored-by: Radek Zikmund <radekzikmund@microsoft.com>

* Don't unload MsQuic from the process (#75441)

* Revert helix queues change (to be done in another PR)

* Code review feedback
@ghost ghost locked as resolved and limited conversation to collaborators Oct 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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