Increase HttpClientHandler.MaxConnectionsPerServer to 64 to improve PM UI performance in Visual Studio #4798

kartheekp-ms · 2022-09-15T02:14:19Z

Bug

Fixes: NuGet/Home#11923

Regression? Last working version:

Description

Thank you @mjolka for identifying this performance improvement and creating the linked issue. For context, HttpClientHandler.MaxConnectionsPerServer is by default 2 in .NET framework but the default value in .NET Core is Int32.MaxValue. Reference https://docs.microsoft.com/en-gb/archive/blogs/timomta/controlling-the-number-of-outgoing-connections-from-httpclient-net-core-or-full-framework. The performance improvements of Visual Studio PM UI mentioned in the linked issue description were great. Hence, in this PR I propose to increase HttpClientHandler.MaxConnectionsPerServer to 64 in .NET Framework code paths but don't change this property in .NET code paths because of the default value i.e., Int32.MaxValue. NuGet.exe on Windows sets max connections limit per endpoint to 64. Hence, I used the same value 64 in this PR. The proposed changes also allow customers to tweak HttpClientHandler.MaxConnectionsPerServer by adding maxHttpRequestsPerSource in NuGet.Config file.

I asked .NET Networking team if there would be any issues upgrading MaxConnectionsPerServer value in .NET Framework code paths. The response received so far from the team seems to be positive https://github.com/orgs/dotnet/teams/ncl/discussions/1. Hence, I went ahead with the changes. Thanks to @drewgillies for taking the time to test a private build with this fix. He mentioned that this fix made a lot of difference to the usability and performance of PM UI in Visual Studio.

Step	With preinstalled PMUI	With PMUI installed from a private build with this fix
Navigate to PMUI for solution, landing on Installed tab	35s	10s
Change to Browse tab	36s	13s
Change back to Installed tab	15s	3s
Change to Updates tab	27s	6s

~~I don't have numbers handy right now, but I noticed great performance improvements in Visual Studio solution restore scenario. I will run a quick test and post the results here.~~

EDIT: I captured few metrics. This PR reduced OrchardCore VS Solution restore by 10 seconds.

VS Solution restore time without this fix.

MaxConnectionsPerSever	MaxHttpRequestsPerSource	Time for Solution restore
2 (Default)	Not set	45 seconds
2 (Default)	64	42 seconds
2 (Default)	128	39 seconds

VS Solution restore time with this fix.

MaxConnectionsPerSever	MaxHttpRequestsPerSource	Time for Solution restore
64	Default (64)	35 seconds
128	128	34 seconds

dotnet restore (7.0.100-preview.6) for OrchardCore solution in my local machine took 32 seconds. This number is close to VS solution restore scenario for default MaxConnectionsPerSever proposed in this PR which is 64.

PR Checklist

nkolev92 · 2022-09-15T19:06:33Z

I think this PR also fixes NuGet/Home#11638 issue. I can't reproduce this issue by installing VSIX from this PR build.

Personally, I have no problem with you marking that issue as being fixed as well.

If anyone from @NuGet/nuget-client disagrees, they should chime in :)

nkolev92 · 2022-09-15T19:09:24Z

src/NuGet.Core/NuGet.Protocol/HttpSource/HttpHandlerResourceV3Provider.cs

@@ -48,6 +48,13 @@ private static HttpHandlerResourceV3 CreateResource(PackageSource packageSource)
                AutomaticDecompression = (DecompressionMethods.GZip | DecompressionMethods.Deflate)
            };

+#if IS_DESKTOP


If people set the MaxHTTPRequestPerSource explicitly we should always respect that.

Doesn't this break that in dotnet.exe?

If customers have configured MaxHttpRequestsPerSource then throttling requests is controlled by following code in both .NET and .NET Framework code paths. we don't change the value to 64 (which is new default).

NuGet.Client/src/NuGet.Core/NuGet.Protocol/HttpSource/HttpSourceResourceProvider.cs

Lines 46 to 49 in a56c42d

else if (source.PackageSource.MaxHttpRequestsPerSource > 0)

{

throttle = SemaphoreSlimThrottle.CreateSemaphoreThrottle(source.PackageSource.MaxHttpRequestsPerSource);

}

The only difference is, in .NET Framework code path if customers configure MaxHttpRequestsPerSource then above logic controls throttling requests and we also set HttpClientHandler.MaxConnectionsPerServer to MaxHttpRequestsPerSource value. The reason is customers may expect NuGet to send that many requests to a source in parallel but at the end because of the default MaxConnectionsPerServer only 2 requests are sent. Whereas in .NET code paths above logic controls throttling requests and we don't MaxConnectionsPerServer because its default value is Int32.MaxValue.

zivkan · 2022-09-15T22:39:34Z

I think this PR also fixes NuGet/Home#11638 issue. I can't reproduce this issue by installing VSIX from this PR build.

If anyone from https://github.com/orgs/NuGet/teams/nuget-client disagrees, they should chime in

I disagree. If VS was hanging when making network calls, that needs to be fixed by not wating for the network to finish on the UI thread. So, it might have been fixed by #4739. If it wasn't, then the duration of the UI delay might be reduced by this PR, but it doesn't fix network calls being made on the UI thread.

nkolev92 · 2022-09-15T23:57:30Z

I think this PR also fixes NuGet/Home#11638 issue. I can't reproduce this issue by installing VSIX from this PR build.

If anyone from https://github.com/orgs/NuGet/teams/nuget-client disagrees, they should chime in

I disagree. If VS was hanging when making network calls, that needs to be fixed by not wating for the network to finish on the UI thread. So, it might have been fixed by #4739. If it wasn't, then the duration of the UI delay might be reduced by this PR, but it doesn't fix network calls being made on the UI thread.

I actually agree with you :D I didn't read that issue in too much detail. It was actually a real hang, with UI freezes and all.
This change simply can't fix that.

kartheekp-ms · 2022-09-16T02:45:56Z

I think this PR also fixes NuGet/Home#11638 issue. I can't reproduce this issue by installing VSIX from this PR build.

If anyone from https://github.com/orgs/NuGet/teams/nuget-client disagrees, they should chime in

I disagree. If VS was hanging when making network calls, that needs to be fixed by not wating for the network to finish on the UI thread. So, it might have been fixed by #4739. If it wasn't, then the duration of the UI delay might be reduced by this PR, but it doesn't fix network calls being made on the UI thread.

I actually agree with you :D I didn't read that issue in too much detail. It was actually a real hang, with UI freezes and all. This change simply can't fix that.

@zivkan @nkolev92 I agree with both of you :). I am unable to reproduce this issue NuGet/Home#11638 even on the internal dogfooding version. Given that @jebriede fixed this UI delay in #4739, I would let him close NuGet/Home#11638 this issue as well.

jebriede · 2022-09-16T17:09:00Z

I think this PR also fixes NuGet/Home#11638 issue. I can't reproduce this issue by installing VSIX from this PR build.

If anyone from https://github.com/orgs/NuGet/teams/nuget-client disagrees, they should chime in

I disagree. If VS was hanging when making network calls, that needs to be fixed by not wating for the network to finish on the UI thread. So, it might have been fixed by #4739. If it wasn't, then the duration of the UI delay might be reduced by this PR, but it doesn't fix network calls being made on the UI thread.

I actually agree with you :D I didn't read that issue in too much detail. It was actually a real hang, with UI freezes and all. This change simply can't fix that.

@zivkan @nkolev92 I agree with both of you :). I am unable to reproduce this issue NuGet/Home#11638 even on the internal dogfooding version. Given that @jebriede fixed this UI delay in #4739, I would let him close NuGet/Home#11638 this issue as well.

@kartheekp-ms The fix I made for #4739 fixed a specific code path from running on the UI thread that was rebuilding cache files on the file system. The fix was not expecting or attempting to resolve all issues where code paths originating from the PMUI were running on the UI thread because each such code path originates from different places. Any other occurrences will need to be addressed separately.

nkolev92 · 2022-09-16T17:21:43Z

I think the problem in NuGet/Home#11638 is that we don't understand what exact codepath no?
Like it could be the same root cause as the one fixed in #4739, but we probably can't know now.

…paths

kartheekp-ms · 2022-09-21T01:13:26Z

I noticed typos in the test names I added in this PR. Hence pushed a new commit updating the test names and added a comment to explain why discard is used to ignore a return value in one of the test cases.

test/NuGet.Core.Tests/NuGet.Protocol.Tests/HttpSourceResourceProviderTests.cs

test/NuGet.Core.Tests/NuGet.Protocol.Tests/HttpHandlerResourceV3ProviderTests.cs

kartheekp-ms · 2022-09-26T22:35:29Z

I have manually queued a PR build by updating the variable to run end to end/apex tests on VS 17.3. CI checks passed for this PR. I am planning to merge this PR today.

Insert 6.4.0-rc.123 into rel/d17.4 on 11/07/2022 23:47:12 * tag '6.4.0.123': (60 commits) fix a logic error that caused AbandonedMutexException while executing migrations (release-6.4.x) (NuGet#4895) unblock source build failing due to fatal: transport 'file' not allowed error (NuGet#4867) (NuGet#4874) Signing: update to August 2022 CTL (NuGet#4791) (NuGet#4850) Merged PR 422933: Prefer BCL Directory create API over helper class (7.0.1xx-rc2) Fix empty combobox when package is not present in project file (NuGet#4844) (NuGet#4848) Fix component detection alert for microsoft.owin package (NuGet#4841) (NuGet#4845) Make release label RC, move to escrow mode Adds special case to include transitive origins in GetInstalledAndTransitivePackagesAsync API (NuGet#4824) Add longPathAware manifest to NuGet.Build.Tasks.Console (NuGet#4830) VsPackageInstallerServices should not post ProjectNotNominatedException faults (NuGet#4814) Skip test GetOrCreateAsync_WithUnhandledExceptionInPlugin_Throws (NuGet#4831) Improve OptProf pipeline job run names (NuGet#4825) Increase HttpClientHandler.MaxConnectionsPerServer to 64 to improve PM UI performance in Visual Studio (NuGet#4798) Suppress CA2213 warnings to unblock dev branch (NuGet#4823) Ensure IsVsOfflineFeed is calculated correctly on 64-bit machines (NuGet#4817) Add better handling of AggregateExceptions in static graph-based restore (NuGet#4809) Add Component Detection task into each pipeline (NuGet#4813) Localizes nuget.exe with default, embedded resource assembly lookup (NuGet#4773) Removes BrowseObjectBase class in NuGet Solution Explorer (NuGet#4807) Improve TryCreateContext (NuGet#4762) ...

kartheekp-ms requested a review from a team as a code owner September 15, 2022 02:14

nkolev92 reviewed Sep 15, 2022

View reviewed changes

nkolev92 previously approved these changes Sep 16, 2022

View reviewed changes

Kartheek Penagamuri added 10 commits September 20, 2022 18:05

update maxrequestspersource to 64 by default for .NET framework code …

bf5c256

…paths

format

73699da

change code

abb7531

fix bug

f659744

fix fail tests

27870ca

code refactoring

e5ebf44

added tests

ed3a703

updated test names

c1528ca

updated tests

7561404

made tests async

5711e83

kartheekp-ms dismissed nkolev92’s stale review via 5711e83 September 21, 2022 01:10

kartheekp-ms force-pushed the dev-kartheekp-ms-vsperf-improvements branch from c1855f3 to 5711e83 Compare September 21, 2022 01:10

zivkan reviewed Sep 21, 2022

View reviewed changes

updated package source url

102af0b

kartheekp-ms requested review from zivkan and nkolev92 September 21, 2022 01:38

zivkan approved these changes Sep 21, 2022

View reviewed changes

nkolev92 approved these changes Sep 21, 2022

View reviewed changes

kartheekp-ms merged commit 451713c into dev Sep 27, 2022

kartheekp-ms deleted the dev-kartheekp-ms-vsperf-improvements branch September 27, 2022 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase HttpClientHandler.MaxConnectionsPerServer to 64 to improve PM UI performance in Visual Studio #4798

Increase HttpClientHandler.MaxConnectionsPerServer to 64 to improve PM UI performance in Visual Studio #4798

kartheekp-ms commented Sep 15, 2022 •

edited

Loading

nkolev92 commented Sep 15, 2022

nkolev92 Sep 15, 2022

kartheekp-ms Sep 16, 2022 •

edited

Loading

zivkan commented Sep 15, 2022

nkolev92 commented Sep 15, 2022

kartheekp-ms commented Sep 16, 2022

jebriede commented Sep 16, 2022 •

edited

Loading

nkolev92 commented Sep 16, 2022

kartheekp-ms commented Sep 21, 2022

kartheekp-ms commented Sep 26, 2022 •

edited

Loading

	else if (source.PackageSource.MaxHttpRequestsPerSource > 0)
	{
	throttle = SemaphoreSlimThrottle.CreateSemaphoreThrottle(source.PackageSource.MaxHttpRequestsPerSource);
	}

Increase HttpClientHandler.MaxConnectionsPerServer to 64 to improve PM UI performance in Visual Studio #4798

Increase HttpClientHandler.MaxConnectionsPerServer to 64 to improve PM UI performance in Visual Studio #4798

Conversation

kartheekp-ms commented Sep 15, 2022 • edited Loading

Bug

Description

PR Checklist

nkolev92 commented Sep 15, 2022

nkolev92 Sep 15, 2022

Choose a reason for hiding this comment

kartheekp-ms Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

zivkan commented Sep 15, 2022

nkolev92 commented Sep 15, 2022

kartheekp-ms commented Sep 16, 2022

jebriede commented Sep 16, 2022 • edited Loading

nkolev92 commented Sep 16, 2022

kartheekp-ms commented Sep 21, 2022

kartheekp-ms commented Sep 26, 2022 • edited Loading

kartheekp-ms commented Sep 15, 2022 •

edited

Loading

kartheekp-ms Sep 16, 2022 •

edited

Loading

jebriede commented Sep 16, 2022 •

edited

Loading

kartheekp-ms commented Sep 26, 2022 •

edited

Loading