Skip to content
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

Fix scan_pkg issue with cycle over-detection #3657

Closed
wants to merge 1 commit into from

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Oct 15, 2023

The main intent here is to ensure that could_be_cycle[pkg] = false happens even when called from the root package, and not just recursively from scan_deps

@vtjnash vtjnash requested a review from Keno October 15, 2023 21:32
@Keno
Copy link
Member

Keno commented Oct 16, 2023

I don't understand why this patch isn't a no-op. On what path does this change behavior?

Copy link
Member Author

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

For the root, I didn't see where it would get set to false eventually in could_be_cycle, but I could have just missed it

@Keno
Copy link
Member

Keno commented Oct 16, 2023

It should get set on return from scan_deps!. I mean, I may have messed it up, but just from what I can tell, this PR is literally equivalent.

@vtjnash
Copy link
Member Author

vtjnash commented Oct 16, 2023

Doesn't the root of the search call scan_pkg and not call scan_deps? I also might not be reading that bit right, this change is purely by visual inspection

@IanButterworth
Copy link
Member

I'm going to hold off backporting #3651 to 1.9 until this is resolved

@Keno
Copy link
Member

Keno commented Oct 17, 2023

Doesn't the root of the search call scan_pkg and not call scan_deps? I also might not be reading that bit right, this change is purely by visual inspection

scan_deps! always gets called on the first call to scan_pkg. Again, as far as I can tell, this PR is a no-op.

@vtjnash
Copy link
Member Author

vtjnash commented Oct 17, 2023

Ah, okay, right. I was confused by having a mutating helper method in a conditional if statement, and that wasn't processing in my mind as being executed always. This PR might remove the need for Box-ing of the two functions for the mutual recursion pairing, but they are functionally equivalent.

@IanButterworth
Copy link
Member

It'd be tidy to resolve this before moving this code to Base.

On a fully precompiled env with DiffEq

This PR

julia> @time Pkg.precompile()
  0.333714 seconds (579.76 k allocations: 77.397 MiB, 1.35% gc time)

julia> @time Pkg.precompile()
  0.340852 seconds (579.76 k allocations: 77.397 MiB, 2.02% gc time)

master

julia> @time Pkg.precompile()
  0.416346 seconds (579.03 k allocations: 77.364 MiB, 23.80% gc time)

julia> @time Pkg.precompile()
  0.332330 seconds (579.03 k allocations: 77.364 MiB)

@KristofferC
Copy link
Member

they are functionally equivalent.

So I don't think this really needs to be "resolved".

@IanButterworth IanButterworth deleted the jn/scan_pkg-circular branch October 23, 2023 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants