Avoid redundant checks in cycle detection

Sources/PackageGraph/PackageGraph+Loading.swift

@@ -869,13 +869,21 @@ fileprivate func findCycle(
 ) rethrows -> (path: [Manifest], cycle: [Manifest])? {
     // Ordered set to hold the current traversed path.
     var path = OrderedCollections.OrderedSet<Manifest>()
+
+    var fullyVisitedManifests = Set<Manifest>()
 
     // Function to visit nodes recursively.
     // FIXME: Convert to stack.
     func visit(
       _ node: GraphLoadingNode,
       _ successors: (GraphLoadingNode) throws -> [GraphLoadingNode]
     ) rethrows -> (path: [Manifest], cycle: [Manifest])? {
+        // Once all successors have been visited, this node cannot participate
+        // in a cycle.
+        if fullyVisitedManifests.contains(node.manifest) {
+            return nil
+        }
+
         // If this node is already in the current path then we have found a cycle.
         if !path.append(node.manifest).inserted {
             let index = path.firstIndex(of: node.manifest)! // forced unwrap safe
@@ -890,6 +898,8 @@ fileprivate func findCycle(
         // No cycle found for this node, remove it from the path.
         let item = path.removeLast()
         assert(item == node.manifest)
+        // Track fully visited nodes
+        fullyVisitedManifests.insert(node.manifest)
         return nil
     }
 

Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift

@@ -87,4 +87,48 @@ class PackageGraphPerfTests: XCTestCasePerf {
             XCTAssertNoDiagnostics(observability.diagnostics)
         }
     }
+
+    func testEfficientCycleDetection() throws {
+        let lastPackageNumber = 20
+        let packageNumberSequence = (1...lastPackageNumber)
+
+        let fs = InMemoryFileSystem(emptyFiles: packageNumberSequence.map({ "/Package\($0)/Sources/Target\($0)/s.swift" }) + ["/PackageA/Sources/TargetA/s.swift"])
+
+        let packageSequence: [Manifest] = try packageNumberSequence.map { (sequenceNumber: Int) -> Manifest in
+            let dependencySequence = sequenceNumber < lastPackageNumber ? Array((sequenceNumber + 1)...lastPackageNumber) : []
+            return Manifest.createFileSystemManifest(
+                name: "Package\(sequenceNumber)",
+                path: try .init(validating: "/Package\(sequenceNumber)"),
+                toolsVersion: .v5_7,
+                dependencies: try dependencySequence.map({ .fileSystem(path: try .init(validating: "/Package\($0)")) }),
+                products: [
+                    try .init(name: "Package\(sequenceNumber)", type: .library(.dynamic), targets: ["Target\(sequenceNumber)"])
+                ],
+                targets: [
+                    try .init(name: "Target\(sequenceNumber)", dependencies: dependencySequence.map { .product(name: "Target\($0)", package: "Package\($0)") })
+                ]
+            )
+        }
+
+        let root = Manifest.createRootManifest(
+            name: "PackageA",
+            path: .init(path: "/PackageA"),
+            toolsVersion: .v5_7,
+            dependencies: try packageNumberSequence.map({ .fileSystem(path: try .init(validating: "/Package\($0)")) }),
+            targets: [try .init(name: "TargetA", dependencies: ["Target1"]) ]
+        )
+
+        let observability = ObservabilitySystem.makeForTesting()
+
+        let N = 1
+        measure {
+            do {
+                for _ in 0..<N {
+                    _ = try loadPackageGraph(fileSystem: fs, manifests: [root] + packageSequence, observabilityScope: observability.topScope)
+                }
+            } catch {
+                XCTFail("Loading package graph is not expected to fail in this test.")
+            }
+        }
+    }
 }