From e542e7a23c01eb74af0bde59b6777218a4ae6fcd Mon Sep 17 00:00:00 2001
From: Max Desiatov <m_desiatov@apple.com>
Date: Wed, 20 Dec 2023 17:58:54 +0000
Subject: [PATCH 1/2] Use `topologicalSort` with `Identifiable` on
 `ResolvedTarget`

Fix fixes a serious performance regression introduced for deep target dependency graphs in https://github.com/apple/swift-package-manager/pull/7160. After converting `ResolvedTarget` to a value type, its synthesized `Hashable` conformance traversed the whole dependency tree multiple times when using `topologicalSort`. This made package resolution seemingly hang for packages that had a lot of nested target graphs.

We already have an implementation of `topologicalSort` on `Identifiable`, thus we should use that instead. I've also added a performance test for this in `PackageGraphPerfTests` to prevent future regressions in this area.
---
 Sources/PackageGraph/GraphLoadingNode.swift   |  4 +
 .../PackageGraph/PackageGraph+Loading.swift   |  1 -
 .../Resolution/ResolvedTarget.swift           | 22 ++---
 .../SPMTestSupport/ResolvedTarget+Mock.swift  | 42 +++++++++
 .../PackageGraphPerfTests.swift               | 20 +++-
 ...tTests.swift => ResolvedTargetTests.swift} | 94 +++++++------------
 6 files changed, 109 insertions(+), 74 deletions(-)
 create mode 100644 Sources/SPMTestSupport/ResolvedTarget+Mock.swift
 rename Tests/PackageGraphTests/{TargetTests.swift => ResolvedTargetTests.swift} (53%)

diff --git a/Sources/PackageGraph/GraphLoadingNode.swift b/Sources/PackageGraph/GraphLoadingNode.swift
index dc28a0d8652..7c66c751be7 100644
--- a/Sources/PackageGraph/GraphLoadingNode.swift
+++ b/Sources/PackageGraph/GraphLoadingNode.swift
@@ -51,3 +51,7 @@ extension GraphLoadingNode: CustomStringConvertible {
         }
     }
 }
+
+extension GraphLoadingNode: Identifiable {
+    public var id: PackageIdentity { self.identity }
+}
diff --git a/Sources/PackageGraph/PackageGraph+Loading.swift b/Sources/PackageGraph/PackageGraph+Loading.swift
index 1e82171692f..46f1bae7fb8 100644
--- a/Sources/PackageGraph/PackageGraph+Loading.swift
+++ b/Sources/PackageGraph/PackageGraph+Loading.swift
@@ -15,7 +15,6 @@ import OrderedCollections
 import PackageLoading
 import PackageModel
 
-import func TSCBasic.topologicalSort
 import func TSCBasic.bestMatch
 
 extension PackageGraph {
diff --git a/Sources/PackageGraph/Resolution/ResolvedTarget.swift b/Sources/PackageGraph/Resolution/ResolvedTarget.swift
index 63e53badfd0..ec0f83c584f 100644
--- a/Sources/PackageGraph/Resolution/ResolvedTarget.swift
+++ b/Sources/PackageGraph/Resolution/ResolvedTarget.swift
@@ -12,8 +12,6 @@
 
 import PackageModel
 
-import func TSCBasic.topologicalSort
-
 /// Represents a fully resolved target. All the dependencies for this target are also stored as resolved.
 public struct ResolvedTarget: Hashable {
     /// Represents dependency of a resolved target.
@@ -72,7 +70,7 @@ public struct ResolvedTarget: Hashable {
 
     /// The name of this target.
     public var name: String {
-        return underlying.name
+        self.underlying.name
     }
 
     /// Returns dependencies which satisfy the input build environment, based on their conditions.
@@ -84,12 +82,12 @@ public struct ResolvedTarget: Hashable {
 
     /// Returns the recursive dependencies, across the whole package-graph.
     public func recursiveDependencies() throws -> [Dependency] {
-        return try TSCBasic.topologicalSort(self.dependencies) { $0.dependencies }
+        try topologicalSort(self.dependencies) { $0.dependencies }
     }
 
     /// Returns the recursive target dependencies, across the whole package-graph.
     public func recursiveTargetDependencies() throws -> [ResolvedTarget] {
-        return try TSCBasic.topologicalSort(self.dependencies) { $0.dependencies }.compactMap { $0.target }
+        try topologicalSort(self.dependencies) { $0.dependencies }.compactMap { $0.target }
     }
 
     /// Returns the recursive dependencies, across the whole package-graph, which satisfy the input build environment,
@@ -97,36 +95,36 @@ public struct ResolvedTarget: Hashable {
     /// - Parameters:
     ///     - environment: The build environment to use to filter dependencies on.
     public func recursiveDependencies(satisfying environment: BuildEnvironment) throws -> [Dependency] {
-        return try TSCBasic.topologicalSort(dependencies(satisfying: environment)) { dependency in
-            return dependency.dependencies.filter { $0.satisfies(environment) }
+        try topologicalSort(dependencies(satisfying: environment)) { dependency in
+            dependency.dependencies.filter { $0.satisfies(environment) }
         }
     }
 
     /// The language-level target name.
     public var c99name: String {
-        return underlying.c99name
+        self.underlying.c99name
     }
 
     /// Module aliases for dependencies of this target. The key is an
     /// original target name and the value is a new unique name mapped
     /// to the name of its .swiftmodule binary.
     public var moduleAliases: [String: String]? {
-      return underlying.moduleAliases
+        self.underlying.moduleAliases
     }
 
     /// Allows access to package symbols from other targets in the package
     public var packageAccess: Bool {
-        return underlying.packageAccess
+        self.underlying.packageAccess
     }
 
     /// The "type" of target.
     public var type: Target.Kind {
-        return underlying.type
+        self.underlying.type
     }
 
     /// The sources for the target.
     public var sources: Sources {
-        return underlying.sources
+        self.underlying.sources
     }
 
     let packageIdentity: PackageIdentity
diff --git a/Sources/SPMTestSupport/ResolvedTarget+Mock.swift b/Sources/SPMTestSupport/ResolvedTarget+Mock.swift
new file mode 100644
index 00000000000..0a95c7aa8e1
--- /dev/null
+++ b/Sources/SPMTestSupport/ResolvedTarget+Mock.swift
@@ -0,0 +1,42 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift open source project
+//
+// Copyright (c) 2014-2017 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See http://swift.org/LICENSE.txt for license information
+// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+import PackageGraph
+import PackageModel
+
+extension ResolvedTarget {
+    public static func mock(
+        packageIdentity: PackageIdentity,
+        name: String,
+        deps: ResolvedTarget...,
+        conditions: [PackageCondition] = []
+    ) -> ResolvedTarget {
+        ResolvedTarget(
+            packageIdentity: packageIdentity,
+            underlying: SwiftTarget(
+                name: name,
+                type: .library,
+                path: .root,
+                sources: Sources(paths: [], root: "/"),
+                dependencies: [],
+                packageAccess: false,
+                swiftVersion: .v4,
+                usesUnsafeFlags: false
+            ),
+            dependencies: deps.map { .target($0, conditions: conditions) },
+            defaultLocalization: nil,
+            supportedPlatforms: [],
+            platformVersionProvider: .init(implementation: .minimumDeploymentTargetDefault)
+        )
+    }
+}
+
diff --git a/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift b/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift
index 1b1c20ff25b..2016be310b3 100644
--- a/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift
+++ b/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift
@@ -2,7 +2,7 @@
 //
 // This source file is part of the Swift open source project
 //
-// Copyright (c) 2014-2017 Apple Inc. and the Swift project authors
+// Copyright (c) 2014-2023 Apple Inc. and the Swift project authors
 // Licensed under Apache License v2.0 with Runtime Library Exception
 //
 // See http://swift.org/LICENSE.txt for license information
@@ -161,4 +161,22 @@ final class PackageGraphPerfTests: XCTestCasePerf {
             }
         }
     }
+
+    func testRecursiveDependencies() throws {
+        var resolvedTarget = ResolvedTarget.mock(packageIdentity: "pkg", name: "t0")
+        for i in 1..<1000 {
+            resolvedTarget = ResolvedTarget.mock(packageIdentity: "pkg", name: "t\(i)", deps: resolvedTarget)
+        }        
+
+        let N = 100
+        measure {
+            do {
+                for _ in 0..<N {
+                    _ = try resolvedTarget.recursiveTargetDependencies()
+                }
+            } catch {
+                XCTFail("Loading package graph is not expected to fail in this test.")
+            }
+        }
+    }
 }
diff --git a/Tests/PackageGraphTests/TargetTests.swift b/Tests/PackageGraphTests/ResolvedTargetTests.swift
similarity index 53%
rename from Tests/PackageGraphTests/TargetTests.swift
rename to Tests/PackageGraphTests/ResolvedTargetTests.swift
index f8b33488149..c24c76d4626 100644
--- a/Tests/PackageGraphTests/TargetTests.swift
+++ b/Tests/PackageGraphTests/ResolvedTargetTests.swift
@@ -2,7 +2,7 @@
 //
 // This source file is part of the Swift open source project
 //
-// Copyright (c) 2014-2018 Apple Inc. and the Swift project authors
+// Copyright (c) 2014-2023 Apple Inc. and the Swift project authors
 // Licensed under Apache License v2.0 with Runtime Library Exception
 //
 // See http://swift.org/LICENSE.txt for license information
@@ -14,49 +14,23 @@ import XCTest
 
 import PackageGraph
 @testable import PackageModel
+import SPMTestSupport
 
-extension ResolvedTarget {
-    fileprivate init(
-        packageIdentity: String,
-        name: String,
-        deps: ResolvedTarget...,
-        conditions: [PackageCondition] = []
-    ) {
-        self.init(
-            packageIdentity: .init(packageIdentity),
-            underlying: SwiftTarget(
-                name: name,
-                type: .library,
-                path: .root,
-                sources: Sources(paths: [], root: "/"),
-                dependencies: [],
-                packageAccess: false,
-                swiftVersion: .v4,
-                usesUnsafeFlags: false
-            ),
-            dependencies: deps.map { .target($0, conditions: conditions) },
-            defaultLocalization: nil,
-            supportedPlatforms: [],
-            platformVersionProvider: .init(implementation: .minimumDeploymentTargetDefault)
-        )
-    }
-}
-
-class TargetDependencyTests: XCTestCase {
+final class ResolvedTargetDependencyTests: XCTestCase {
     func test1() throws {
-        let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
-        let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
-        let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2)
+        let t1 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t1")
+        let t2 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
+        let t3 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t3", deps: t2)
 
         XCTAssertEqual(try t3.recursiveTargetDependencies(), [t2, t1])
         XCTAssertEqual(try t2.recursiveTargetDependencies(), [t1])
     }
 
     func test2() throws {
-        let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
-        let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
-        let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2, t1)
-        let t4 = ResolvedTarget(packageIdentity: "pkg", name: "t4", deps: t2, t3, t1)
+        let t1 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t1")
+        let t2 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
+        let t3 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t3", deps: t2, t1)
+        let t4 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t4", deps: t2, t3, t1)
 
         XCTAssertEqual(try t4.recursiveTargetDependencies(), [t3, t2, t1])
         XCTAssertEqual(try t3.recursiveTargetDependencies(), [t2, t1])
@@ -64,10 +38,10 @@ class TargetDependencyTests: XCTestCase {
     }
 
     func test3() throws {
-        let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
-        let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
-        let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2, t1)
-        let t4 = ResolvedTarget(packageIdentity: "pkg", name: "t4", deps: t1, t2, t3)
+        let t1 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t1")
+        let t2 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
+        let t3 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t3", deps: t2, t1)
+        let t4 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t4", deps: t1, t2, t3)
 
         XCTAssertEqual(try t4.recursiveTargetDependencies(), [t3, t2, t1])
         XCTAssertEqual(try t3.recursiveTargetDependencies(), [t2, t1])
@@ -75,10 +49,10 @@ class TargetDependencyTests: XCTestCase {
     }
 
     func test4() throws {
-        let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
-        let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
-        let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2)
-        let t4 = ResolvedTarget(packageIdentity: "pkg", name: "t4", deps: t3)
+        let t1 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t1")
+        let t2 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
+        let t3 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t3", deps: t2)
+        let t4 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t4", deps: t3)
 
         XCTAssertEqual(try t4.recursiveTargetDependencies(), [t3, t2, t1])
         XCTAssertEqual(try t3.recursiveTargetDependencies(), [t2, t1])
@@ -86,12 +60,12 @@ class TargetDependencyTests: XCTestCase {
     }
 
     func test5() throws {
-        let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
-        let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
-        let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2)
-        let t4 = ResolvedTarget(packageIdentity: "pkg", name: "t4", deps: t3)
-        let t5 = ResolvedTarget(packageIdentity: "pkg", name: "t5", deps: t2)
-        let t6 = ResolvedTarget(packageIdentity: "pkg", name: "t6", deps: t5, t4)
+        let t1 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t1")
+        let t2 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
+        let t3 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t3", deps: t2)
+        let t4 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t4", deps: t3)
+        let t5 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t5", deps: t2)
+        let t6 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t6", deps: t5, t4)
 
         // precise order is not important, but it is important that the following are true
         let t6rd = try t6.recursiveTargetDependencies()
@@ -108,12 +82,12 @@ class TargetDependencyTests: XCTestCase {
     }
 
     func test6() throws {
-        let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
-        let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
-        let t3 = ResolvedTarget(packageIdentity: "pkg", name: "t3", deps: t2)
-        let t4 = ResolvedTarget(packageIdentity: "pkg", name: "t4", deps: t3)
-        let t5 = ResolvedTarget(packageIdentity: "pkg", name: "t5", deps: t2)
-        let t6 = ResolvedTarget(
+        let t1 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t1")
+        let t2 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
+        let t3 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t3", deps: t2)
+        let t4 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t4", deps: t3)
+        let t5 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t5", deps: t2)
+        let t6 = ResolvedTarget.mock(
             packageIdentity: "pkg",
             name: "t6",
             deps: t4,
@@ -135,10 +109,10 @@ class TargetDependencyTests: XCTestCase {
     }
 
     func testConditions() throws {
-        let t1 = ResolvedTarget(packageIdentity: "pkg", name: "t1")
-        let t2 = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
-        let t2NoConditions = ResolvedTarget(packageIdentity: "pkg", name: "t2", deps: t1)
-        let t2WithConditions = ResolvedTarget(
+        let t1 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t1")
+        let t2 = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
+        let t2NoConditions = ResolvedTarget.mock(packageIdentity: "pkg", name: "t2", deps: t1)
+        let t2WithConditions = ResolvedTarget.mock(
             packageIdentity: "pkg",
             name: "t2",
             deps: t1,

From 8e167f1602a8ea68dadbca4414aaf43281b01d64 Mon Sep 17 00:00:00 2001
From: Max Desiatov <m_desiatov@apple.com>
Date: Thu, 21 Dec 2023 13:32:43 +0000
Subject: [PATCH 2/2] Reduce number of iterations in
 `testRecursiveDependencies`

---
 Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift b/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift
index 2016be310b3..6cd5f755c80 100644
--- a/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift
+++ b/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift
@@ -168,7 +168,7 @@ final class PackageGraphPerfTests: XCTestCasePerf {
             resolvedTarget = ResolvedTarget.mock(packageIdentity: "pkg", name: "t\(i)", deps: resolvedTarget)
         }        
 
-        let N = 100
+        let N = 10
         measure {
             do {
                 for _ in 0..<N {