diff --git a/Sources/PackageGraph/PackageGraph+Loading.swift b/Sources/PackageGraph/PackageGraph+Loading.swift index 293d7d6d5cd..c84db183208 100644 --- a/Sources/PackageGraph/PackageGraph+Loading.swift +++ b/Sources/PackageGraph/PackageGraph+Loading.swift @@ -872,6 +872,8 @@ fileprivate func findCycle( ) rethrows -> (path: [Manifest], cycle: [Manifest])? { // Ordered set to hold the current traversed path. var path = OrderedCollections.OrderedSet() + + var fullyVisitedManifests = Set() // Function to visit nodes recursively. // FIXME: Convert to stack. @@ -879,6 +881,12 @@ fileprivate func findCycle( _ 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 @@ -893,6 +901,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 } diff --git a/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift b/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift index 124562c5702..e76c10310a7 100644 --- a/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift +++ b/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("/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..