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

[Bug]: Visual Studio Installed Packages page is slow to load #11923

Closed
mjolka opened this issue Jun 28, 2022 · 6 comments · Fixed by NuGet/NuGet.Client#4798
Closed

[Bug]: Visual Studio Installed Packages page is slow to load #11923

mjolka opened this issue Jun 28, 2022 · 6 comments · Fixed by NuGet/NuGet.Client#4798
Assignees
Labels
Category:Quality Week Issues that should be considered for quality week PerfWins Priority:2 Issues for the current backlog. Product:VS.Client Tenet:Performance Performance issues Type:Bug
Milestone

Comments

@mjolka
Copy link

mjolka commented Jun 28, 2022

NuGet Product Used

Visual Studio Package Management UI

Product Version

Visual Studio 2022

Worked before?

No response

Impact

It bothers me. A fix would be nice

Repro Steps & Context

The Installed packages page in the Visual Studio Package Management UI can be slow to load.

Part of the reason for this slowness is HttpClientHandler's MaxConnectionsPerServer setting, which by default is 2.

Increasing this value results in a much faster load time. In the attached video, setting MaxConnectionsPerServer to 20 reduces the time taken from ~42 seconds to ~14 seconds.

max.connections.per.server.mp4

This can be further improved by changing PackageMetadataResourceV3.GetMetadataAsync to make its API requests concurrently. In the attached video, the time taken is further reduced to ~5 seconds.

concurrent.metadata.mp4

The diff for these changes can be found here: mjolka/NuGet.Client@6da1317

This is the project file I used to test with:

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="AWSSDK.Core" Version="3.7.12.3" />
    <PackageReference Include="AWSSDK.S3" Version="3.7.9.19" />
    <PackageReference Include="AWSSDK.SecretsManager" Version="3.7.2.59" />
    <PackageReference Include="AWSSDK.SecretsManager.Caching" Version="1.0.4" />
    <PackageReference Include="MassTransit" Version="8.0.3" />
    <PackageReference Include="MassTransit.RabbitMQ" Version="8.0.3" />
    <PackageReference Include="Microsoft.EntityFrameworkCore" Version="6.0.6" />
    <PackageReference Include="Npgsql.EntityFrameworkCore.PostgreSQL" Version="6.0.5" />
    <PackageReference Include="Swashbuckle.AspNetCore" Version="6.2.3" />
  </ItemGroup>

</Project>

Verbose Logs

No response

@erdembayar
Copy link
Contributor

erdembayar commented Jul 6, 2022

Thank you for raising this issue.
Actually, I played on same perf improvements before and know where the bottlenecks. For 1st one improvement was noticeable on beefy machines but makes experience slow for non-beefy machines. Also, there's concerns about how it affects other VS connections. Also 2nd one had has ordering correctness and race condition issue. I'll work on this after #11529 is implemented.

@erdembayar erdembayar added Tenet:Performance Performance issues and removed Triage:Untriaged labels Jul 6, 2022
@erdembayar erdembayar self-assigned this Jul 6, 2022
@erdembayar erdembayar added Priority:2 Issues for the current backlog. Product:VS.Client labels Jul 6, 2022
@mjolka
Copy link
Author

mjolka commented Jul 6, 2022

Hi @erdembayar,

Thanks for the response.

My solution at work has 83 packages. It takes 3 minutes for all 906 NuGet API requests to complete. Just increasing MaxConnectionsPerServer from 2 to 64 (which is what NuGet.exe uses) reduces that time to 15-20 seconds.

Waiting 3 minutes for those requests to complete really impacts my experience using NuGet.

As a compromise, would you consider increasing MaxConnectionsPerServer for "beefy" machines? Or making it configurable, so that people can choose a value that works for them?

In the meantime, as a workaround, I've found that by running a proxy (e.g. Fiddler with "Capture HTTPS CONNECTs" enabled) seems to bypass the MaxConnectionsPerServer setting ([1], [2]) and results in a similar performance improvement.

Can you please elaborate on the ordering correctness and race condition issue? Thanks.

@erdembayar
Copy link
Contributor

Hi @erdembayar,

Thanks for the response.

My solution at work has 83 packages. It takes 3 minutes for all 906 NuGet API requests to complete. Just increasing MaxConnectionsPerServer from 2 to 64 (which is what NuGet.exe uses) reduces that time to 15-20 seconds.

Waiting 3 minutes for those requests to complete really impacts my experience using NuGet.

As a compromise, would you consider increasing MaxConnectionsPerServer for "beefy" machines? Or making it configurable, so that people can choose a value that works for them?

In the meantime, as a workaround, I've found that by running a proxy (e.g. Fiddler with "Capture HTTPS CONNECTs" enabled) seems to bypass the MaxConnectionsPerServer setting ([1], [2]) and results in a similar performance improvement.

Changing value here might affect another VS components because we share same resource (but mono or nuget.exe is not inside VS so they can go for 64 connections without issue), that is why didn't propose to change it last time during perf investigation. I'll ask again then let you know.

Can you please elaborate on the ordering correctness and race condition issue? Thanks.

This is normal list, so when updating from multithread might cause issue, also tasks might not complete in correct order, I know what the fix is, only doesn't have time to get on it:

@mjolka
Copy link
Author

mjolka commented Jul 6, 2022

@erdembayar Thanks for the explanation. In the diff, results isn't updated from multiple threads, it's updated in a loop after all the tasks have completed:

https://github.com/mjolka/NuGet.Client/blob/6da131780ccf74c1d4de9306804c295f5462e57b/src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs#L133-L141

Task.WhenAll guarantees that the results are in the same order as the supplied tasks.

The pattern is very similar to the one found here: https://github.com/NuGet/NuGet.Client/blob/f24bad0668193ce21a1db8cabd1ce95ba509c7f0/src/NuGet.Core/NuGet.Protocol/DependencyInfo/RegistrationUtility.cs#L65-L103

I'll ask again then let you know.

Thanks, I look forward to hearing the response!

@erdembayar
Copy link
Contributor

@erdembayar Thanks for the explanation. In the diff, results isn't updated from multiple threads, it's updated in a loop after all the tasks have completed:

Problem is here. https://github.com/mjolka/NuGet.Client/blob/6da131780ccf74c1d4de9306804c295f5462e57b/src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs#L278

https://github.com/mjolka/NuGet.Client/blob/6da131780ccf74c1d4de9306804c295f5462e57b/src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs#L133-L141

Task.WhenAll guarantees that the results are in the same order as the supplied tasks.

The pattern is very similar to the one found here: https://github.com/NuGet/NuGet.Client/blob/f24bad0668193ce21a1db8cabd1ce95ba509c7f0/src/NuGet.Core/NuGet.Protocol/DependencyInfo/RegistrationUtility.cs#L65-L103

Yes, fixing orderings is not hard, same as you just found, it needs little bit more work on top of Task.WhenAll.

@kartheekp-ms kartheekp-ms added the Category:Quality Week Issues that should be considered for quality week label Sep 6, 2022
@kartheekp-ms
Copy link
Contributor

Related - #11638

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Quality Week Issues that should be considered for quality week PerfWins Priority:2 Issues for the current backlog. Product:VS.Client Tenet:Performance Performance issues Type:Bug
Projects
None yet
4 participants