Improved performance of calculation of PrivateAssets for transitively pinned centrally managed dependencies #4954

marcin-krystianc · 2022-11-29T12:05:22Z

Bug

Regression? Last working version: 6.0.400

Description

Existing code enumerated parent nodes in an inefficient fashion, it could yield the same node many times. Another performance issue was a linear scan to get the dependency node for the given parent node.
It is being fixed by:

using a HashSet to avoid traversing the same nodes multiple times
using a dictionary lookup instead of a linear scan.

PR Checklist

…ansitiveDependencies

…rcink-20221128-assets_flow_for_project_and_packages

…ess whether the parent is a project or a package

…ink-20221123-CentralTransitiveDependencies # Conflicts: # src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

ghost · 2022-12-06T13:01:15Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 90 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

jeffkl · 2023-02-22T23:48:07Z

@marcin-krystianc what's the status of this? I'd like to get this in the next release if there's a large performance increase.

marcin-krystianc · 2023-02-23T09:04:53Z

@marcin-krystianc what's the status of this? I'd like to get this in the next release if there's a large performance increase.

I needed previous PRs to be merged. Now I can resolve remaining conflicts and fine tune it. It should be ready to review soon.

…alTransitiveDependencies # Conflicts: # src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs # test/NuGet.Core.Tests/NuGet.Commands.Test/IncludeTypeTests.cs # test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests.cs

marcin-krystianc · 2023-02-23T17:31:14Z

@jeffkl It is ready for a review. I've tested the restore time using the test scenario from the NuGet/Home#12269 and now the restore time is more or less the same as it was for 6.0.400.

jeffkl · 2023-02-23T21:47:03Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

@@ -517,6 +517,8 @@ private IEnumerable<LibraryDependency> GetLibraryDependenciesForCentralTransitiv
        {
            foreach (GraphNode<RemoteResolveResult> rootNode in targetGraph.Graphs)
            {
+                var dependencyDictionary = rootNode.Item.Data.Dependencies.ToDictionary(x => x.Name, x => x, StringComparer.OrdinalIgnoreCase);


Are there cases where this dictionary won't end up being used? On line 525 nodes can be skipped, so perhaps don't initialize this dictionary until its needed on line 538?

Yeah, it is possible that the dictionary will not be eventually used but it is not very likely scenario. I don't have strong opinion here..

jeffkl · 2023-02-23T21:47:18Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

-                        }
-
-                        suppressParent &= parentDependency.SuppressParent;
+                        var dependency = dependencyDictionary[dependencyNode.Key.Name];


Use explicit type instead of var please.

jeffkl · 2023-02-23T21:50:59Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

                    {
-                        yield return outerNode;
+                        // It's what we are looking for


Please clarify this comment that the node is a parent

jeffkl

Thanks for the contribution. I ran the changes under the debugger and everything seems to still work. Would it be possible to get some benchmarks just so we know how much faster this is?

nkolev92

Thanks for this fix.

A few suggestions to reduce allocations.

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

marcin-krystianc · 2023-02-28T12:06:09Z

Thanks for the contribution. I ran the changes under the debugger and everything seems to still work. Would it be possible to get some benchmarks just so we know how much faster this is?

Sure, just got some measurements for https://github.com/marcin-krystianc/TestSolutions/tree/master/LargeAppWithPrivatePackagesCentralisedNGBVRemoved on my dev laptop (multiple runs, but posting the best result only):

6.0.400, Time Elapsed 00:00:26.15
7.0.100, Time Elapsed 00:00:57.18
7.0.100 (with this fix), Time Elapsed 00:00:28.28

I see that there is still some noticeable difference when compared to 6.0.400, but I think it is justified by the extra work needed to correctly calculate effective PrivateAssets value for central transitive dependencies.

…ations

jeffkl · 2023-03-01T23:01:21Z

@marcin-krystianc something broke with our PR build pipeline but is now fixed, can you please rebase your change onto latest dev branch and force push?

nkolev92 · 2023-03-02T01:41:56Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

@@ -525,23 +530,21 @@ private IEnumerable<LibraryDependency> GetLibraryDependenciesForCentralTransitiv
                        continue;
                    }

+                    if (dependencyDictionary == null)
+                    {
+                        dependencyDictionary = rootNode.Item.Data.Dependencies.ToDictionary(x => x.Name, x => x, StringComparer.OrdinalIgnoreCase);


The fact that GraphNode doesn't implement hashcode made me a bit nervous, but I think it's ok in this case, as the data itself will never be duplicated.

Hmm, not sure what was your concern really. Was it about HashSet<GraphNode<T>> for visited nodes? I think it is ok that it doesn't have proper equality comparer. We care about not visiting the same graph node twice, but it is ok when there are multiple nodes representing same project or package.

…alTransitiveDependencies

marcin-krystianc · 2023-03-02T11:31:03Z

@marcin-krystianc something broke with our PR build pipeline but is now fixed, can you please rebase your change onto latest dev branch and force push?

Done

davkean · 2023-05-31T11:51:45Z

@marcin-krystianc Nice work on this. Looking our allocation telemetry, I can see this addressed the majority of the issues on this path.

marcin-krystianc added 7 commits November 23, 2022 15:10

improved performance and fixed the GetLibraryDependenciesForCentralTr…

b10a62f

…ansitiveDependencies

Look only at direct references when flattening the graph

2c31b02

Merge branch 'dev' into marcink-20221123-CentralTransitiveDependencies

620547c

Merge branch 'marcink-20221124-FlattenDependencyTypesUnified' into ma…

cc4c3cd

…rcink-20221128-assets_flow_for_project_and_packages

PrivateAssets for central transitive dependencies should flow regardl…

f1a069e

…ess whether the parent is a project or a package

Effective PrivateAssets should be an intersection of parent dependencies

5d3fbd3

Merge branch 'marcink-20221129-private_assets_intersection' into marc…

8df4d82

…ink-20221123-CentralTransitiveDependencies # Conflicts: # src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

ghost added the Community PRs created by someone not in the NuGet team label Nov 29, 2022

marcin-krystianc mentioned this pull request Nov 29, 2022

Effective PrivateAssets of centrally managed transitive dependencies should be an intersection of parent dependencies #4953

Merged

8 tasks

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Dec 6, 2022

jeffkl mentioned this pull request Dec 6, 2022

[Epic]: Central Package Management improvements for 17.5 NuGet/Home#12301

Closed

4 tasks

jeffkl mentioned this pull request Feb 6, 2023

[Epic]: Central Package Management improvements for 17.6 NuGet/Home#12413

Closed

6 tasks

ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Feb 22, 2023

marcin-krystianc marked this pull request as ready for review February 23, 2023 15:34

marcin-krystianc requested a review from a team as a code owner February 23, 2023 15:34

improve node enumeration

82d3109

jeffkl added the Approved for CI label Feb 23, 2023

jeffkl reviewed Feb 23, 2023

View reviewed changes

pr remarks

6e4e758

marcin-krystianc force-pushed the marcink-20221123-CentralTransitiveDependencies branch from 03dae82 to 6e4e758 Compare February 24, 2023 11:01

marcin-krystianc requested a review from jeffkl February 24, 2023 11:03

jeffkl previously approved these changes Feb 24, 2023

View reviewed changes

jeffkl added Approved for CI and removed Approved for CI labels Feb 24, 2023

jeffkl added the Approved for CI label Feb 24, 2023

nkolev92 reviewed Feb 24, 2023

View reviewed changes

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs Show resolved Hide resolved

Use reusable collection for graph traversal algorithm to reduce alloc…

0e3e1ba

…ations

marcin-krystianc dismissed jeffkl’s stale review via 0e3e1ba February 28, 2023 14:59

jeffkl approved these changes Feb 28, 2023

View reviewed changes

jeffkl added Approved for CI and removed Approved for CI labels Feb 28, 2023

nkolev92 approved these changes Mar 2, 2023

View reviewed changes

Merge remote-tracking branch 'origin/dev' into marcink-20221123-Centr…

7f1306f

…alTransitiveDependencies

jeffkl added Approved for CI and removed Approved for CI labels Mar 2, 2023

jeffkl enabled auto-merge (squash) March 2, 2023 16:52

jeffkl merged commit c911884 into NuGet:dev Mar 5, 2023

marcin-krystianc deleted the marcink-20221123-CentralTransitiveDependencies branch March 6, 2023 07:17

marcin-krystianc mentioned this pull request Apr 4, 2023

Active NuGet restore regression G-Research/NuPerfMonitor#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved performance of calculation of PrivateAssets for transitively pinned centrally managed dependencies #4954

Improved performance of calculation of PrivateAssets for transitively pinned centrally managed dependencies #4954

marcin-krystianc commented Nov 29, 2022 •

edited

Loading

ghost commented Dec 6, 2022

jeffkl commented Feb 22, 2023

marcin-krystianc commented Feb 23, 2023 •

edited

Loading

marcin-krystianc commented Feb 23, 2023

jeffkl Feb 23, 2023

marcin-krystianc Feb 24, 2023

jeffkl Feb 23, 2023

jeffkl Feb 23, 2023

jeffkl left a comment

nkolev92 left a comment

marcin-krystianc commented Feb 28, 2023 •

edited

Loading

jeffkl commented Mar 1, 2023

nkolev92 Mar 2, 2023 •

edited

Loading

marcin-krystianc Mar 2, 2023

marcin-krystianc commented Mar 2, 2023

davkean commented May 31, 2023

Improved performance of calculation of PrivateAssets for transitively pinned centrally managed dependencies #4954

Improved performance of calculation of PrivateAssets for transitively pinned centrally managed dependencies #4954

Conversation

marcin-krystianc commented Nov 29, 2022 • edited Loading

Bug

Description

PR Checklist

ghost commented Dec 6, 2022

jeffkl commented Feb 22, 2023

marcin-krystianc commented Feb 23, 2023 • edited Loading

marcin-krystianc commented Feb 23, 2023

jeffkl Feb 23, 2023

Choose a reason for hiding this comment

marcin-krystianc Feb 24, 2023

Choose a reason for hiding this comment

jeffkl Feb 23, 2023

Choose a reason for hiding this comment

jeffkl Feb 23, 2023

Choose a reason for hiding this comment

jeffkl left a comment

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

marcin-krystianc commented Feb 28, 2023 • edited Loading

jeffkl commented Mar 1, 2023

nkolev92 Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

marcin-krystianc Mar 2, 2023

Choose a reason for hiding this comment

marcin-krystianc commented Mar 2, 2023

davkean commented May 31, 2023

marcin-krystianc commented Nov 29, 2022 •

edited

Loading

marcin-krystianc commented Feb 23, 2023 •

edited

Loading

marcin-krystianc commented Feb 28, 2023 •

edited

Loading

nkolev92 Mar 2, 2023 •

edited

Loading