Avoid redundant checks in cycle detection #5839

fcanas · 2022-10-25T14:15:28Z

Dramatically improve speed of cycle detection for large or well-connected manifest graphs.

Motivation:

Loading a project with a large or well-connected package graph in Xcode can be very slow.

Xcode uses Workspace.loadPackageGraph(rootPath:...), which calls into
calls into findCycle(). findCycle has a bug leading to slower than linear cycle detection.

Modifications:

There is a note to convert to use an algorithm tracking traversal with a stack, which would allow for linear time cycle detection. But given the output of findCycle is the path to first the cycle, and the cycle itself, not all the elements in a strongly connected subgraph, it's enough to memoize fully explored nodes.

Once all of a Manifests outbound edges have been traversed for cycles, it's not possible for it to participate in a cycle. So if a fully-explored Manifest is encountered again in a cycle check, exit early.

Without such an early exit, cycle detection is quadratic. This change makes it linear.

Result:

I made a tool to generate and profile the loading of package graphs through the same path as Xcode here.

These are the results of profiling package loading a small range of number of packages.

Number of packages	apple/spm	memoized
22	17.9s	0.2s
23	35.8s	0.2s
24	88.1s	0.2s
25	160.5s	0.2s
26	323.4s	0.2s

Once all of a Manifests outbound edges have been traversed for cycles, it's not possible for it to participate in a cycle. So if a fully-explored Manifest is encountered again in a cycle check, exit early. Without such an early exit, cycle detection is quadratic. This change makes it linear.

Sources/PackageGraph/PackageGraph+Loading.swift

MaxDesiatov · 2022-10-25T15:23:47Z

@swift-ci please smoke test

neonichu · 2022-10-25T17:02:58Z

@swift-ci please smoke test linux

fcanas · 2022-10-25T18:38:33Z

This change can also apply cleanly to the 5.7 branch. Given the impact, I would hope this would be considered for consideration in a future release of Xcode.

tomerd · 2022-10-25T20:07:16Z

looks great, thank you @fcanas!

is it possible to add a test to make sure this does not regress?

fcanas · 2022-10-25T20:20:46Z

@tomerd I'm happy to add tests.

I'm guessing I should add fixtures somewhere in Fixtures/DependencyResolution. Do you have any guidance as to whether this is "External" or "Internal"? And do you have any preference on the naming?

The tests will otherwise look like what I've shared in https://github.com/fcanas/manifest-cycle-bug, except with fixed fixtures to follow the existing pattern. Unless you'd like me to share the fixture generation as well.

tomerd · 2022-10-25T20:56:44Z

This change can also apply cleanly to the 5.7 branch. Given the impact, I would hope this would be considered for consideration in a future release of Xcode.

please prepare a 5.7 PR once this is merged, and we can look for an opportunity if and when such arise

I'm guessing I should add fixtures somewhere in Fixtures/DependencyResolution. Do you have any guidance as to whether this is "External" or "Internal"? And do you have any preference on the naming?

The tests will otherwise look like what I've shared in https://github.com/fcanas/manifest-cycle-bug, except with fixed fixtures to follow the existing pattern. Unless you'd like me to share the fixture generation as well.

Fixtures is one way, probably under "Miscellaneous".

Alternatively you can synthesize a resolution unit test (see WorkspaceTests), assuming there are clear side effects we can use to validate this

fcanas · 2022-10-25T21:27:30Z

I've added a performance test following as many of the patterns I could see for such tests, including putting the test in a subclass of XCTestCasePerf, and using the fixture loading utility.

I'm not sure how this project manages baselines for performance measurements. If this test were to run on master, the time would be several minutes on almost any machine. On an M1 MacBook Air, it's just under 5 seconds.

I'm happy to reshape or move what I've added.

Tests/WorkspaceTests/WorkspaceTests.swift

tomerd · 2022-10-26T00:02:43Z

Tests/WorkspaceTests/WorkspaceTests.swift

+			let N = 1
+			measure {
+				for _ in 1..<N {
+					try! workspace.loadPackageGraph(rootPath: rootPackagePath, observabilityScope: observability.topScope)


try! -> try

All the other uses of measure in the codebase use try!, so I was following that pattern. But I personally do disagree with crashing a test process when we can fail and complete the test suite.

Done.

tomerd · 2022-10-26T00:04:15Z

Tests/WorkspaceTests/WorkspaceTests.swift

@@ -12355,6 +12355,25 @@ final class WorkspaceTests: XCTestCase {
    }
 }

+final class WorkspacePerformanceTests: XCTestCasePerf {


perhaps we should move this to PackageGraphPerfTests? @neonichu opinions?

Whatever you prefer.

The XCTestCasePerf seems to be used for excluding certain performance tests unless specified. This case could be rolled up into the same class in this file and would just be run regardless of being specifically a performance test run.

If it's sufficiently standardized, this test function could check for the TSC_ENABLE_PERF_TESTS environment variable and succeed early if it's not present. But reaching into TSC for arbitrary constants isn't something I would do without being asked to.

Or this new test case class could be in a new file, Tests/WorkspaceTests/WorkspacePerformanceTests.swift.

Yah, I think testing that at the package graph rather than the workspace level sounds right to me, but I am not sure we have infra there to load from fixtures.

fixture is in SPMTestSupport so should be fine? if not, we can leave it here

Once I looked at testing the package graph more directly instead of via the workspace, doing it with generated Manifests rather than fixtures seemed viable, and more in keeping with the other Package tests.

Updated.

abertelrud · 2022-10-26T22:42:31Z

@swift-ci please smoke test

neonichu · 2022-10-27T00:25:14Z

@swift-ci please smoke test

tomerd

thank you @fcanas

fcanas · 2022-10-27T02:19:48Z

I've created a pull request for the 5.7 branch

neonichu · 2022-10-27T04:06:48Z

Interesting error from the self-hosted job:

Undefined symbols for architecture x86_64:
  "_$s12SPMBuildCore23ProductBuildDescriptionMp", referenced from:
      _symbolic ______p 12SPMBuildCore23ProductBuildDescriptionP in BuildOperationBuildSystemDelegateHandler.swift.o
      _symbolic Say______pG 12SPMBuildCore23ProductBuildDescriptionP in BuildOperationBuildSystemDelegateHandler.swift.o
  "_$s12SPMBuildCore23ProductBuildDescriptionPAAE6binary8TSCBasic12AbsolutePathVvg", referenced from:
      _$s5Build0A11DescriptionV4plan13swiftCommands0d8FrontendE0013testDiscoveryE00g10EntryPointE004copyE018pluginDescriptionsAcA0A4PlanC_SDySS15LLBuildManifest17SwiftCompilerToolVGSDySSAM0qfS0VGSDySSAM04TesthS0VGSDySSAM0tijS0VGSDySSAM04CopyS0VGSayAA06PluginB0CGtKcfc12SPMBuildCore05BuiltT7ProductVA4_0zaB0_pXEfU3_ in BuildOperationBuildSystemDelegateHandler.swift.o

neonichu · 2022-10-27T04:10:06Z

@swift-ci please smoke test macOS

tomerd · 2022-10-27T16:59:50Z

/Users/ec2-user/jenkins/workspace/swift-package-manager-with-xcode-self-hosted-PR-osx/branch-main/swiftpm/Tests/PackageGraphPerformanceTests/PackageGraphPerfTests.swift:97:61: error: unable to infer complex closure return type; add explicit type to disambiguate
        let packageSequence = try packageNumberSequence.map { sequenceNumber in
                                                            ^
                                                                             -> <#Result#> 
[18/66] Compiling PackageGraphPerformanceTests DependencyResolverPerfTests.swift
[19/67] Merging module PackagePluginAPITests
[20/67] Merging module PackageLoadingPerformanceTests
[2

@fcanas some of the CI machines use older compiler which needs a bit of help with type inference. should be easy to fix by adding the type signature

neonichu · 2022-10-27T19:55:56Z

@swift-ci please smoke test

fcanas requested review from abertelrud, neonichu, tomerd and elsh as code owners October 25, 2022 14:15

Convert tabs to spaces

e5b2bd7

MaxDesiatov reviewed Oct 25, 2022

View reviewed changes

Sources/PackageGraph/PackageGraph+Loading.swift Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

Fix indentation

4898ac7

neonichu approved these changes Oct 25, 2022

View reviewed changes

Add performance test for Workspace package loading

03e5a88

tomerd reviewed Oct 26, 2022

View reviewed changes

Tests/WorkspaceTests/WorkspaceTests.swift Outdated Show resolved Hide resolved

tomerd reviewed Oct 26, 2022

View reviewed changes

Don't disable error propagation

61b4b0e

Move test to PackageGraph and remove fixtures

7d7fd15

tomerd approved these changes Oct 27, 2022

View reviewed changes

Add type signature to help inference

017300e

tomerd merged commit 5639797 into swiftlang:main Oct 28, 2022

tomerd mentioned this pull request Oct 28, 2022

Improve speed of finding dependency cycles #5628

Closed

Avoid redundant checks in cycle detection #5839

Avoid redundant checks in cycle detection #5839

Uh oh!

Conversation

fcanas commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation:

Modifications:

Result:

Uh oh!

Uh oh!

This comment was marked as outdated.

MaxDesiatov commented Oct 25, 2022

Uh oh!

neonichu commented Oct 25, 2022

Uh oh!

fcanas commented Oct 25, 2022

Uh oh!

tomerd commented Oct 25, 2022

Uh oh!

fcanas commented Oct 25, 2022

Uh oh!

tomerd commented Oct 25, 2022

Uh oh!

fcanas commented Oct 25, 2022

Uh oh!

Uh oh!

tomerd Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

fcanas Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

tomerd Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

fcanas Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

neonichu Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

tomerd Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

fcanas Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

abertelrud commented Oct 26, 2022

Uh oh!

neonichu commented Oct 27, 2022

Uh oh!

tomerd left a comment

Choose a reason for hiding this comment

Uh oh!

fcanas commented Oct 27, 2022

Uh oh!

neonichu commented Oct 27, 2022

Uh oh!

neonichu commented Oct 27, 2022

Uh oh!

tomerd commented Oct 27, 2022

Uh oh!

neonichu commented Oct 27, 2022

Uh oh!

Uh oh!

fcanas commented Oct 25, 2022 •

edited

Loading