GetInstalledAndTransitivePackagesAsync transitive origins computation can be optional by dominoFire · Pull Request #4788 · NuGet/NuGet.Client

dominoFire · 2022-09-06T18:48:09Z

Bug

Fixes:
https://github.com/NuGet/Client.Engineering/issues/1808
https://github.com/NuGet/Client.Engineering/issues/1780

Regression? Yes.
Last working version: VS 17.2

Description

Adds a new parameter to GetInstalledAndTransitivePackagesAsync (gitpa) to decide whether or not calculate transitive origins
Use new gitpa API only on installed tab in project PM UI with experiment enabled
Removes transitive origins computation in GetInstalledPackagesAsync API
Special casing of each PM UI scenario to only get installed packages when required
Added a special case to get targets list (assets file, expensive operation) when this is null when computing transitive origins.

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added
- OR
- Test exception: Fixed current tests. Need more tests
- OR
- N/A
Documentation
- Documentation PR or issue filled
- OR
- N/A: No product changes.

Validation

Profiled PM UI first load with a project with 10 installed packages and a thousand (1118) transitive packages. I used PerfView and captured CPU samples.

Perfview Histograms below shows a smaller CPU time usage. The Makespan metric is the duration of first and last sample captured in PerfView. A smaller Makespan indicates it took less time to run gitpa API. However, some cases, we had a larger Makespan, but histograms indicate a lower CPU usage. Measurements are in miliseconds

Browse tab

Case	First	Last	Makespan	Improvement	PercImprovement	PerfView Histogram
Baseline	3,147.42	7,448.17	4,300.75			`____________00____0_____0_____0_`
Fix 1	3,081.87	6,079.17	2,997.30	-0.303075764	0.696924236	`___________ooo___oo____o________`

Consolidade tab in Solution PM UI

First	Last	Description	Makespan	PerfView Histogram
4,379.66	8,469.06	Baseline	4,089.41	`____________00___0___o_oo_______`
3,784.31	8,950.83	Fix 1	5,166.52	`________0o______o_oo____________`

Installed tab

First	Last	Description	Makespan	Improvement	PercImprovement
3,918.87	8,042.28	Baseline	4123.41
4849.036	7238.275	Fix1,attempt1	2389.239	-0.4205672	0.4205672
3184.073	7192.473	Fix1,attempt2	4008.4	-0.027891963	0.027891963
4,196.14	10,075.73	Fix1,attempt3	5879.588	0.425904288	-0.425904288

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Projects/PackageReferenceProject.cs

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Services/NuGetPackageSearchService.cs

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Services/NuGetProjectManagerService.cs

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Projects/PackageReferenceProject.cs

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Projects/IPackageReferenceProject.cs

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Projects/PackageReferenceProject.cs

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Common/IProjectContextInfoExtensions.cs

jebriede · 2022-09-08T21:47:54Z

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Services/NuGetPackageSearchService.cs

            {
                if (isSolution)
                {
+                    // We only need the installed packages? What about recommender


I believe Recommender is only for the project-level PMUI. Ask @birgithi to be sure.

Correct. The recommender is only project level at this time. We've considered including solution level, but we can deal with that when we decide to include it.

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Services/NuGetPackageSearchService.cs

...t.Clients.Tests/NuGet.PackageManagement.VisualStudio.Test/CpsPackageReferenceProjectTests.cs

Comment wording Co-authored-by: Jean-Pierre Briedé <jebriede@microsoft.com>

aortiz-msft · 2022-09-16T23:44:58Z

src/NuGet.Clients/NuGet.VisualStudio.Internal.Contracts/INuGetProjectManagerService.cs

        ValueTask<IInstalledAndTransitivePackages> GetInstalledAndTransitivePackagesAsync(
            IReadOnlyCollection<string> projectIds,
            CancellationToken cancellationToken);
+        /// <summary>


NIT: Add an empty line.

I will address this in a follow-up issue. NuGet/Home#12098

nkolev92

One small comment. It's ok to have fewer APIs in code completely owned by us.

nkolev92 · 2022-09-16T23:37:16Z

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Projects/IPackageReferenceProject.cs

+        /// <param name="includeTransitiveOrigins">Set it to <c>true</c> to get transitive origins in transitive packages list</param>
+        /// <param name="token">Cancellation token</param>
+        /// <returns>A <see cref="ProjectPackages"/>A struct with two lists: installed and transitive packages</returns>
+        public Task<ProjectPackages> GetInstalledAndTransitivePackagesAsync(bool includeTransitiveOrigins, CancellationToken token);


nit: Given that this isn't a client visible public API, we don't have to duplicate the API :) We can just have 1.

I will address this in a follow-up issue. NuGet/Home#12098

jebriede · 2022-09-16T20:29:23Z

...t.Clients.Tests/NuGet.PackageManagement.VisualStudio.Test/CpsPackageReferenceProjectTests.cs

+        }
+
+        [Fact]
+        public async Task GetInstalledAndTransitivePackagesAsync_TransitiveOriginsParamSetToFalse_ReturnsInstalledAndTransitivePackagesOnlyAsync()


What about _ReturnsPackagesWithoutTransitiveOriginsAsync as a suffix?

Its current phrasing indicates it's only returning installed and transitive packages, but that's all we can return, there are no other package types at this time. The transitive origins are a child of transitive packages, not a sibling, so this wording would make it clearer that the test is validating all packages are returned without any transitive origins. Thoughts?

I will address this in a follow-up issue. NuGet/Home#12098

jebriede · 2022-09-16T20:32:58Z

...t.Clients.Tests/NuGet.PackageManagement.VisualStudio.Test/CpsPackageReferenceProjectTests.cs

+        }
+
+        [Fact]
+        public async Task GetInstalledAndTransitivePackagesAsync_TransitiveOriginsParamSetToTrue_ReturnsTransitiveOriginsAsync()


For consistency and clarity, I recommend _ReturnsPackagesWithTransitiveOriginsAsync for the suffix here.

I will address this in a follow-up issue. NuGet/Home#12098

jebriede · 2022-09-16T20:34:04Z

...t.Clients.Tests/NuGet.PackageManagement.VisualStudio.Test/CpsPackageReferenceProjectTests.cs

+            // Assert
+            Assert.NotEmpty(packages.InstalledPackages);
+            Assert.NotEmpty(packages.TransitivePackages);
+            Assert.All(packages.TransitivePackages, pkg => Assert.NotEmpty(pkg.TransitiveOrigins));


Assert the collection of transitive origins contains what you expect rather than just checking for it being non-empty, unless this is already covered in another test. If it's in another test, can you link it here? Thanks!

I will address this in a follow-up issue. NuGet/Home#12098

jebriede · 2022-09-16T20:36:09Z

...t.Clients.Tests/NuGet.PackageManagement.VisualStudio.Test/CpsPackageReferenceProjectTests.cs

+                // Setup
+                var projectName = "project1";
+                string projectFullPath = Path.Combine(rootDir.SolutionRoot, projectName + ".csproj");
+
+                // Project
+                var projectCache = new ProjectSystemCache();
+                IVsProjectAdapter projectAdapter = Mock.Of<IVsProjectAdapter>();
+                CpsPackageReferenceProject project = CreateCpsPackageReferenceProject(projectName, projectFullPath, projectCache);
+
+                ProjectNames projectNames = GetTestProjectNames(projectFullPath, projectName);
+                PackageSpec packageSpec = GetPackageSpec(projectName, projectFullPath, "[2.0.0, )");
+
+                // Restore info
+                DependencyGraphSpec projectRestoreInfo = ProjectTestHelpers.GetDGSpecFromPackageSpecs(packageSpec);
+                projectCache.AddProjectRestoreInfo(projectNames, projectRestoreInfo, new List<IAssetsLogMessage>());
+                projectCache.AddProject(projectNames, projectAdapter, project).Should().BeTrue();
+
+                // Package directories
+                var sources = new List<PackageSource>();
+                var packagesDir = new DirectoryInfo(Path.Combine(rootDir.SolutionRoot, "globalPackages"));
+                var packageSource = new DirectoryInfo(rootDir.PackageSource);
+                packagesDir.Create();
+                packageSource.Create();
+                sources.Add(new PackageSource(packageSource.FullName));
+
+                var logger = new TestLogger();
+                var request = new TestRestoreRequest(packageSpec, sources, packagesDir.FullName, logger)
+                {
+                    LockFilePath = Path.Combine(rootDir.SolutionRoot, "project.assets.json")
+                };
+
+                await SimpleTestPackageUtility.CreateFullPackageAsync(packageSource.FullName, "packageB", "1.0.0");
+                await SimpleTestPackageUtility.CreateFullPackageAsync(
+                    packageSource.FullName,
+                    "packageA",
+                    "2.15.3",
+                    new Packaging.Core.PackageDependency[]
+                    {
+                        new Packaging.Core.PackageDependency("packageB", VersionRange.Parse("1.0.0"))
+                    });
+
+                var command = new RestoreCommand(request);
+                RestoreResult result = await command.ExecuteAsync();
+                await result.CommitAsync(logger, CancellationToken.None);


Without looking too closely, this setup code looks very similar to the setup in PrepareTestProjectAsync. Can we use that here instead or refactor so we can share most of it?

I will address this in a follow-up issue. NuGet/Home#12098

Superseeded by last review

…putation can be optional (#4788)" This reverts commit abaa72f.

Fernando Aguilar added 5 commits September 6, 2022 19:18

Created gitpa API with optional transitive origins

036033a

Initial working draft

a54ca9c

Tests: Fixed 1 test

55edd69

Tests: Fixed tests

f060194

Tests: Added tests

25a4029

dominoFire force-pushed the dev-dominofire-gitpa-perf-fix branch from b49888a to 25a4029 Compare September 7, 2022 02:30

dominoFire marked this pull request as ready for review September 7, 2022 02:36

dominoFire requested a review from a team as a code owner September 7, 2022 02:36

lbussell mentioned this pull request Sep 7, 2022

Update TFM to net7.0 for source-build #4742

Merged

8 tasks

Fixed identation

a80f788

aortiz-msft previously requested changes Sep 8, 2022

View reviewed changes

martinrrm reviewed Sep 9, 2022

View reviewed changes

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Projects/PackageReferenceProject.cs Outdated Show resolved Hide resolved

jebriede previously requested changes Sep 9, 2022

View reviewed changes

Fernando Aguilar and others added 10 commits September 15, 2022 12:18

Apply suggestions from code review

8daf6a8

Comment wording Co-authored-by: Jean-Pierre Briedé <jebriede@microsoft.com>

Addressed feedback

70c43af

Renamed param

68385a5

Fix naming

53c44c4

Renamed test param

a1a9bf1

Renamed params

f804fc7

nits

3216916

More nits

3ff969d

Fixed tests

4dfa76b

Min-changeset

62dbd07

dominoFire requested review from a team, aortiz-msft, jebriede and martinrrm September 16, 2022 01:47

Updated casing

d92ad3d

aortiz-msft reviewed Sep 16, 2022

View reviewed changes

nkolev92 approved these changes Sep 16, 2022

View reviewed changes

jebriede reviewed Sep 17, 2022

View reviewed changes

jebriede self-requested a review September 17, 2022 01:15

dominoFire merged commit abaa72f into dev Sep 17, 2022

dominoFire deleted the dev-dominofire-gitpa-perf-fix branch September 17, 2022 01:51

dominoFire mentioned this pull request Sep 17, 2022

Remove GetInstalledAndTransitivePackagesAsync() API in PackageRefenceProject NuGet/Home#12098

Closed

6 tasks

dominoFire pushed a commit that referenced this pull request Sep 20, 2022

Revert "GetInstalledAndTransitivePackagesAsync transitive origins com…

b1c25c0

…putation can be optional (#4788)" This reverts commit abaa72f.

dominoFire pushed a commit that referenced this pull request Sep 20, 2022

Revert "GetInstalledAndTransitivePackagesAsync transitive origins com…

40e75ec

…putation can be optional (#4788)" This reverts commit abaa72f.

Conversation

dominoFire commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Description

PR Checklist

Validation

Browse tab

Consolidade tab in Solution PM UI

Installed tab

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkolev92 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jebriede Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jebriede Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jebriede Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dominoFire commented Sep 6, 2022 •

edited

Loading

jebriede Sep 16, 2022 •

edited

Loading

jebriede Sep 16, 2022 •

edited

Loading

jebriede Sep 16, 2022 •

edited

Loading