[5.7] Avoid redundant checks in cycle detection #5847
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. Convert tabs to spaces Fix indentation Add performance test for Workspace package loading Don't disable error propagation Move test to PackageGraph and remove fixtures
fcanas
requested review from
abertelrud,
neonichu,
tomerd and
elsh
as code owners
|
@swift-ci please smoke test |
|
My mistake. The tests need adjustment from the main branch version. I'll push a fix soon. |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
tomerd
changed the title
|
@tomerd Is there anything else I can do to move this change along? |
|
thanks @fcanas, the 5.7 release process is carefully managed. we will merge this if / when there is an opportunity to take it forward |
tomerd
approved these changes
Nov 30, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 intocalls into
findCycle().findCyclehas 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
findCycleis 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 slower than linear. 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.