Skip to content

Improve speed of finding dependency cycles #5628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

mpsnp
Copy link

@mpsnp mpsnp commented Jun 25, 2022

Significantly improve speed of finding cycles in dependency tree

Motivation:

At the moment, if dependencies are composed to layers, depending on graph size, it can take minutes and even hours to validate that there are no cycles. Take a look at this issue and a PR in swift-tools-support-core.

Modifications:

Cache already checked graph nodes and reuse results in subsequent visits.

Result:

This modification improved complexity to O(n) instead of O(n!). So package graph of 100 layers with 100 nodes in each layer is checked in 0.2 seconds instead of hours.

@mpsnp mpsnp changed the title Improve speed of finding cycles Improve speed of finding dependency cycles Jun 25, 2022
Copy link
Contributor

@tomerd tomerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the verbosity change intentional?

@mpsnp
Copy link
Author

mpsnp commented Jun 25, 2022

@tomerd excuse me, don’t get what you mean. Which verbosity?

@mpsnp
Copy link
Author

mpsnp commented Jun 25, 2022

Okay, you mean to use validManifests instead of validPackages?

@mpsnp
Copy link
Author

mpsnp commented Jun 25, 2022

Actually I think I’ll replace this func by the one from swift-tools-support-core.

@tomerd
Copy link
Contributor

tomerd commented Jun 26, 2022

@mpsnp i mean the changes to the plugin runner. they seem accidental / merge issue?

@tomerd
Copy link
Contributor

tomerd commented Jun 26, 2022

wrt to using tools-support-core, our goal is to deprecate that library, so leaning on the SwiftPM version is preferred

@tomerd
Copy link
Contributor

tomerd commented Jun 27, 2022

@mpsnp interestingly, GitHub showed different diff before (on my phone, so maybe the GitHub iOS App was caching something). this looks fine, just needs a unit test. wrt to using tools-support-core, our goal is to deprecate that library, so leaning on the SwiftPM version is preferred

@mpsnp
Copy link
Author

mpsnp commented Jun 28, 2022

Yeah, that's why i was confused :)

Will update PR today, with unit tests, small naming fixes and checks that nothing else uses function from tools-support-core

@tomerd
Copy link
Contributor

tomerd commented Oct 28, 2022

looks like this was done via #5839

@tomerd
Copy link
Contributor

tomerd commented Oct 28, 2022

thanks @mpsnp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants