Eliminate unnecessary graph-walk restarts for stale transitive children in DependencyGraphResolver

Co-authored-by: jeffkl <[email protected]>

Author: Copilot

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

@@ -895,25 +895,33 @@ private async Task<Dictionary<LibraryDependencyIndex, ResolvedDependencyGraphIte
             // Cache the result of IsNewerThanNET10 since it is invariant for the entire method
             bool enablePruningWarnings = IsNewerThanNET10(projectTargetFramework.FrameworkName);
 
-        // Used to start over when a dependency has multiple descendants of an item to be evicted.
+        // A restart is needed when evicting one item invalidates a previous eviction (a cascade eviction).
+        //
+        // Consider this graph:
         //
         // Project
         // ├── A 1.0.0
-        // │   └── B 1.0.0
-        // │       └── C 1.0.0
-        // │           └── D 1.0.0
-        // └── X 2.0.0
-        //     └── Y 2.0.0
-        //         └── G 2.0.0
-        //             └── B 2.0.0
+        // │   └── Q 1.0.0
+        // └── B 1.0.0
+        //     └── Q 2.0.0
+        // └── C 2.0.0
+        //     └── B 2.0.0
+        //
         // The items are processed in the following order:
-        // Chose A 1.0.0 and X 1.0.0
-        // Chose B 1.0.0 and Y 2.0.0
-        // Chose C 1.0.0 and G 2.0.0
-        // Chose D 1.0.0 and B 2.0.0, but B 2.0.0 should evict C 1.0.0 and D 1.0.0
+        // A 1.0.0 and B 1.0.0 and C 2.0.0 are chosen.
+        // Q 1.0.0 is chosen initially, but Q 2.0.0 (from B 1.0.0's subtree) evicts it.
+        // evictions[Q1] records a winning path through B 1.0.0.
+        // B 2.0.0 (from C 2.0.0's subtree) evicts B 1.0.0.
         //
-        // In this case, the entire walk is started over and B 1.0.0 is left out of the graph, leading to C 1.0.0 and D 1.0.0 also being left out.
+        // Since Q 2.0.0's winning path went through B 1.0.0, which is now itself evicted,
+        // Q 1.0.0's eviction is no longer valid and must be reconsidered.
+        // Because Q 1.0.0 was already dequeued and rejected, the walk must restart from the beginning.
+        // On the next pass, evictions[B1] prevents B 1.0.0 from being processed, so Q 2.0.0 is never
+        // reached and Q 1.0.0 is chosen without competition.
         //
+        // Transitive children of an evicted item that are already in resolvedDependencyGraphItems (for
+        // example C and D in B 1.0.0's subtree) are handled without a restart by removing them directly
+        // from resolvedDependencyGraphItems; items still in the queue are skipped by the lazy path check.
         StartOver:
             restartCount++;
 
@@ -931,7 +939,6 @@ private async Task<Dictionary<LibraryDependencyIndex, ResolvedDependencyGraphIte
 
                 GraphItem<RemoteResolveResult> currentGraphItem = await currentDependencyGraphItem.GetGraphItemAsync(_request.Project.RestoreMetadata, projectTargetFramework.PackagesToPrune, enablePruningWarnings, isRootProject, targetGraphName, _logger);
 
-                LibraryDependencyTarget typeConstraint = currentDependencyGraphItem.LibraryDependency.LibraryRange.TypeConstraint;
                 if (evictions.TryGetValue(currentDependencyGraphItem.LibraryRangeIndex, out (LibraryRangeIndex[] Path, LibraryDependencyIndex DependencyIndex, LibraryDependencyTarget TypeConstraint) eviction))
                 {
                     // If we evicted this same version previously, but the type constraint of currentRef is more stringent (package), then do not skip the current item - this is the one we want.
@@ -1059,14 +1066,28 @@ private async Task<Dictionary<LibraryDependencyIndex, ResolvedDependencyGraphIte
                         // Record an eviction for the item we are replacing.  The eviction path is for the current item.
                         LibraryRangeIndex evictedLibraryRangeIndex = chosenResolvedItem.LibraryRangeIndex;
 
-                        bool shouldStartOver = false;
+                        // Record the eviction immediately so that:
+                        // 1. The evicted range is skipped if re-encountered at dequeue time (LibraryRangeIndex key check).
+                        // 2. Items still in the queue whose path passes through the evicted item are lazily skipped.
+                        evictions[evictedLibraryRangeIndex] = (DependencyGraphItemIndexer.CreatePathToRef(currentDependencyGraphItem.Path, currentDependencyGraphItem.LibraryRangeIndex), currentDependencyGraphItem.LibraryDependencyIndex, chosenResolvedItem.LibraryDependency.LibraryRange.TypeConstraint);
 
-                        // To "evict" a chosen item, we need to also remove all of its transitive children from the chosen list.
+                        // Check whether any previously-recorded evictions have a winning path that passes through
+                        // the item we just evicted.  Those evictions are now invalid because the ancestor that caused
+                        // them is itself evicted.  The previously-rejected items must be re-evaluated, but they are
+                        // no longer in the queue, so the entire walk must restart.
                         HashSet<LibraryRangeIndex>? evicteesToRemove = default;
 
                         foreach (KeyValuePair<LibraryRangeIndex, (LibraryRangeIndex[] Path, LibraryDependencyIndex DependencyIndex, LibraryDependencyTarget TypeConstraint)> evictee in evictions)
                         {
-                            // See if the evictee is a descendant of the evicted item
+                            // Skip the eviction entry we just recorded above; it was added before this loop
+                            // started, so the dictionary enumerator will reach it, but it is not a pre-existing
+                            // eviction and its path will never contain evictedLibraryRangeIndex by definition.
+                            if (evictee.Key == evictedLibraryRangeIndex)
+                            {
+                                continue;
+                            }
+
+                            // See if the evictee's winning path passes through the item we just evicted
                             if (evictee.Value.Path.Contains(evictedLibraryRangeIndex))
                             {
                                 // if evictee.Key (depIndex) == currentDepIndex && evictee.TypeConstraint == ExternalProject --> Don't remove it.  It must remain evicted.
@@ -1086,28 +1107,35 @@ private async Task<Dictionary<LibraryDependencyIndex, ResolvedDependencyGraphIte
                             foreach (LibraryRangeIndex evicteeToRemove in evicteesToRemove)
                             {
                                 evictions.Remove(evicteeToRemove);
-
-                                // Indicate that we can't simply evict this item and instead we need to start over knowing that this item should be skipped
-                                shouldStartOver = true;
                             }
+
+                            // The previously-rejected items that are now un-evicted are no longer in the queue,
+                            // so the walk must restart from the beginning.
+                            goto StartOver;
                         }
 
-                        foreach (KeyValuePair<LibraryDependencyIndex, ResolvedDependencyGraphItem> chosenItem in resolvedDependencyGraphItems)
+                        // No cascade eviction requires a restart.  Remove any already-resolved items whose ancestry
+                        // passes through the evicted item; they are stale because the evicted item is no longer in
+                        // the graph.  Items still in the queue that are children of the evicted item will be skipped
+                        // lazily by the path check at dequeue time.
+                        List<LibraryDependencyIndex>? resolvedItemsToRemove = null;
+
+                        foreach (KeyValuePair<LibraryDependencyIndex, ResolvedDependencyGraphItem> resolvedItem in resolvedDependencyGraphItems)
                         {
-                            if (chosenItem.Value.Path.Contains(evictedLibraryRangeIndex))
+                            if (resolvedItem.Value.Path.Contains(evictedLibraryRangeIndex))
                             {
-                                // Indicate that we can't simply evict this item and instead we need to start over knowing that this item should be skipped
-                                shouldStartOver = true;
-                                break;
+                                resolvedItemsToRemove ??= new List<LibraryDependencyIndex>(capacity: 4);
+
+                                resolvedItemsToRemove.Add(resolvedItem.Key);
                             }
                         }
 
-                        // Add the eviction to be used later; children of the evicted item that remain in the queue will be skipped lazily via the path check above
-                        evictions[evictedLibraryRangeIndex] = (DependencyGraphItemIndexer.CreatePathToRef(currentDependencyGraphItem.Path, currentDependencyGraphItem.LibraryRangeIndex), currentDependencyGraphItem.LibraryDependencyIndex, chosenResolvedItem.LibraryDependency.LibraryRange.TypeConstraint);
-
-                        if (shouldStartOver)
+                        if (resolvedItemsToRemove != null)
                         {
-                            goto StartOver;
+                            foreach (LibraryDependencyIndex key in resolvedItemsToRemove)
+                            {
+                                resolvedDependencyGraphItems.Remove(key);
+                            }
                         }
 
                         // Add the item to the list of chosen items