Unload MsQuic after checking for QUIC support to free resources. #74749

rzikm · 2022-08-29T11:39:29Z

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

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

ghost · 2022-08-29T11:39:44Z

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

Issue Details

Fixes #74629.

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

Author:	rzikm
Assignees:	-
Labels:	`area-System.Net.Http`
Milestone:	-

rzikm · 2022-08-29T12:10:17Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs

+        NativeLibrary.TryLoad($"{Interop.Libraries.MsQuic}.{MsQuicVersion.Major}", typeof(MsQuicApi).Assembly, DllImportSearchPath.AssemblyDirectory, out msQuicHandle) ||
+        NativeLibrary.TryLoad(Interop.Libraries.MsQuic, typeof(MsQuicApi).Assembly, DllImportSearchPath.AssemblyDirectory, out msQuicHandle);
+
+    internal static bool TryOpenMsQuic(IntPtr msQuicHandle, out QUIC_API_TABLE* apiTable)


We could alternatively use [LibraryImport] for MsQuicOpenVersion and MsQuicClose, it might clean up the code a bit at the cost of not being able to unload the lib from the process I think (if we care about that)

They already have imports in:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/msquic_generated.cs

Line 2593 in bf47ac0

internal static extern int MsQuicOpenVersion([NativeTypeName("uint32_t")] uint Version, [NativeTypeName("const void **")] void** QuicApi);

I'm not an expert on this, but we control which version of libmsquic we're loading in TryLoadMsQuic and we do an attempt to load it that would not fail if the lib is not present. I'm not sure we can achieve the same with the import. Relevant history: #49973

Anyway, we might at least use nameof(MsQuicOpenVersion) from the generated interop. Though we lived with the string until now, so it's certainly not critical.

ManickaP · 2022-08-29T13:43:49Z

Are the threads started by opening MsQuic: MsQuicOpenVersion or by opening registration: RegistrationOpen?

rzikm · 2022-08-29T14:06:48Z

Are the threads started by opening MsQuic: MsQuicOpenVersion or by opening registration: RegistrationOpen?

It is the MsQuicOpenVersion and MsQuicClose calls that change the number of threads visible.

wfurt

LGTM

ManickaP

LGTM, modulo some nits.

ManickaP · 2022-09-01T08:49:34Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs

@@ -47,7 +48,8 @@ private MsQuicApi(QUIC_API_TABLE* apiTable)
        }
    }

-    internal static MsQuicApi Api { get; } = null!;
+    internal static Lazy<MsQuicApi> _api = new Lazy<MsQuicApi>(AllocateMsQuicApi);


private readonly?

ManickaP · 2022-09-01T08:50:02Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs

            {
                return;
            }

            try
            {
+                // check version


Uber nitty nit 😄

Suggested change

// check version

// Check version

ManickaP · 2022-09-01T08:50:50Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs

+        throw new Exception("Failed to create MsQuicApi instance");
+    }
+
+    internal static bool TryLoadMsQuic(out IntPtr msQuicHandle) =>


Should those be private?

ManickaP · 2022-09-01T08:55:55Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs

+        NativeLibrary.TryLoad($"{Interop.Libraries.MsQuic}.{MsQuicVersion.Major}", typeof(MsQuicApi).Assembly, DllImportSearchPath.AssemblyDirectory, out msQuicHandle) ||
+        NativeLibrary.TryLoad(Interop.Libraries.MsQuic, typeof(MsQuicApi).Assembly, DllImportSearchPath.AssemblyDirectory, out msQuicHandle);
+
+    internal static bool TryOpenMsQuic(IntPtr msQuicHandle, out QUIC_API_TABLE* apiTable)


They already have imports in:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Interop/msquic_generated.cs

Line 2593 in bf47ac0

internal static extern int MsQuicOpenVersion([NativeTypeName("uint32_t")] uint Version, [NativeTypeName("const void **")] void** QuicApi);

I'm not an expert on this, but we control which version of libmsquic we're loading in TryLoadMsQuic and we do an attempt to load it that would not fail if the lib is not present. I'm not sure we can achieve the same with the import. Relevant history: #49973

Anyway, we might at least use nameof(MsQuicOpenVersion) from the generated interop. Though we lived with the string until now, so it's certainly not critical.

rzikm · 2022-09-01T14:44:30Z

/backport to release/7.0

github-actions · 2022-09-01T14:44:50Z

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

…es. (dotnet#74749)" This reverts commit 7a45201.

…es. (#74749)" (#74984) This reverts commit 7a45201.

rzikm · 2022-09-02T13:52:30Z

Reverted by #74984

… resources. (#74749)" (#74984)" This reverts commit 953f524.

* 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 <[email protected]>

…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 <[email protected]>

…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 <[email protected]> * Don't unload MsQuic from the process (#75441) * Revert helix queues change (to be done in another PR) * Code review feedback

Unload MsQuic after checking for QUIC support to free resources.

fee1c6d

dotnet-issue-labeler bot added the area-System.Net.Http label Aug 29, 2022

ghost assigned rzikm Aug 29, 2022

rzikm commented Aug 29, 2022

View reviewed changes

rzikm marked this pull request as ready for review August 29, 2022 12:10

rzikm requested review from ManickaP and wfurt August 29, 2022 12:11

wfurt approved these changes Aug 31, 2022

View reviewed changes

Unload MsQuic unconditionally

bf47ac0

ManickaP approved these changes Sep 1, 2022

View reviewed changes

Code review feedback

162c331

rzikm merged commit 7a45201 into dotnet:main Sep 1, 2022

github-actions bot mentioned this pull request Sep 1, 2022

[release/7.0] Unload MsQuic after checking for QUIC support to free resources. #74931

Closed

wfurt mentioned this pull request Sep 1, 2022

[Tests] System.Net.Quic.Tests System.Exception : Failed to create MsQuicApi instance #74952

Closed

rzikm added a commit to rzikm/dotnet-runtime that referenced this pull request Sep 2, 2022

Revert "Unload MsQuic after checking for QUIC support to free resourc…

36f24b5

…es. (dotnet#74749)" This reverts commit 7a45201.

jkotas pushed a commit that referenced this pull request Sep 2, 2022

Revert "Unload MsQuic after checking for QUIC support to free resourc…

953f524

…es. (#74749)" (#74984) This reverts commit 7a45201.

karelz added this to the 8.0.0 milestone Sep 3, 2022

wfurt added a commit that referenced this pull request Sep 6, 2022

Revert "Revert "Unload MsQuic after checking for QUIC support to free…

5e20ff9

… resources. (#74749)" (#74984)" This reverts commit 953f524.

wfurt mentioned this pull request Sep 6, 2022

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

Merged

This was referenced Sep 9, 2022

[release/7.0] Unload MsQuic after checking for QUIC support to free resources (#75163) #75330

Closed

Don't unload MsQuic from the process #75441

Merged

jkotas mentioned this pull request Sep 13, 2022

System.Net.Http.Functional - HTTP/3 timeouts (mostly on NativeAOT) #75493

Closed

ghost locked as resolved and limited conversation to collaborators Oct 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unload MsQuic after checking for QUIC support to free resources. #74749

Unload MsQuic after checking for QUIC support to free resources. #74749

rzikm commented Aug 29, 2022 •

edited

Loading

ghost commented Aug 29, 2022

rzikm Aug 29, 2022

ManickaP Sep 1, 2022

ManickaP commented Aug 29, 2022

rzikm commented Aug 29, 2022

wfurt left a comment

ManickaP left a comment

ManickaP Sep 1, 2022

ManickaP Sep 1, 2022

ManickaP Sep 1, 2022

ManickaP Sep 1, 2022

rzikm commented Sep 1, 2022

github-actions bot commented Sep 1, 2022

rzikm commented Sep 2, 2022

Unload MsQuic after checking for QUIC support to free resources. #74749

Unload MsQuic after checking for QUIC support to free resources. #74749

Conversation

rzikm commented Aug 29, 2022 • edited Loading

ghost commented Aug 29, 2022

rzikm Aug 29, 2022

Choose a reason for hiding this comment

ManickaP Sep 1, 2022

Choose a reason for hiding this comment

ManickaP commented Aug 29, 2022

rzikm commented Aug 29, 2022

wfurt left a comment

Choose a reason for hiding this comment

ManickaP left a comment

Choose a reason for hiding this comment

ManickaP Sep 1, 2022

Choose a reason for hiding this comment

ManickaP Sep 1, 2022

Choose a reason for hiding this comment

ManickaP Sep 1, 2022

Choose a reason for hiding this comment

ManickaP Sep 1, 2022

Choose a reason for hiding this comment

rzikm commented Sep 1, 2022

github-actions bot commented Sep 1, 2022

rzikm commented Sep 2, 2022

rzikm commented Aug 29, 2022 •

edited

Loading