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

Replace checkHomeUnitsClosed with a faster implementation #4109

Merged
merged 4 commits into from
Apr 23, 2024

Conversation

wz1000
Copy link
Collaborator

@wz1000 wz1000 commented Mar 1, 2024

GHC gives a default "main" home unit, and it is difficult to avoid this as
each ghc session must have an active home unit at all times, but in a multiple
component session, there is no other good choice for a default unit. We could
pick one arbitrarily, but this is ugly and complicates the code a lot.

We never use this default "main" unit for anything, as the GHC sessions
corresponding to any file/component have the active unit set to the correct one
for that component.

When checking for home unit closure, we must make sure to include only the
actual units in the project, not the bogus "main" unit that GHC forces us to
have. Including the main unit seems to make the checkHomeUnitsClosed function loop
forever for some reason.

Use a faster implementation of checkHomeUnitsClosed
GHC had an implementation of this function, but it was horribly inefficient
We should move back to the GHC implementation on compilers where
https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12162 is included

Fixes #4046

@michaelpj
Copy link
Collaborator

Hooray!

Please put the reasoning into the code! Can we have a test case? It would be nice to know what its going wrong with the checkHomeUnitsClosed function - it would be much better for it to throw or fail somehow rather than looping!

@wz1000
Copy link
Collaborator Author

wz1000 commented Mar 1, 2024

I'm investigating that currently, it seems like checkHomeUnitsClosed is just extremely slow - it traverses all paths in the unit dependency graph, which seems to work when your dependency closure is small but quickly grows to be massive.

@michaelpj
Copy link
Collaborator

I'm investigating that currently, it seems like checkHomeUnitsClosed is just extremely slow - it traverses all paths in the unit dependency graph, which seems to work when your dependency closure is small but quickly grows to be massive.

That definitely seems bad. I would have thought this was a standard graph problem of some kind 🤔

@wz1000
Copy link
Collaborator Author

wz1000 commented Mar 1, 2024

I've put up a patch here, I will copy the implementation to ghcide so we can use it on released GHCs https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12162

@wz1000 wz1000 force-pushed the wip/multi-loop-2 branch 2 times, most recently from 1555164 to 8b3a381 Compare March 4, 2024 10:42
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Idea looks great, I do have a couple of questions, though.

@michaelpj
Copy link
Collaborator

I'm assuming we're going to wait for the GHC MR to land so we can incorporate any comments from upstream.

@ch1bo
Copy link

ch1bo commented Apr 18, 2024

Seems like https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12162 will get merged / backported. Can we get this proper workaround for < 9.10 compilers now? :)

@michaelpj
Copy link
Collaborator

Yes, we should be able to copy in the final code now.

@wz1000 wz1000 changed the title When checking for home unit closure, only include the actual home units in the project. Replace checkHomeUnitsClosed with a faster implementation Apr 22, 2024
GHC had an implementation of this function, but it was horribly inefficient
We should move back to the GHC implementation on compilers where
https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12162 is included

Fixes #4046
@wz1000 wz1000 added the merge me Label to trigger pull request merge label Apr 23, 2024
ghcide/src/Development/IDE/GHC/Compat/Core.hs Outdated Show resolved Hide resolved
ghcide/session-loader/Development/IDE/Session.hs Outdated Show resolved Hide resolved
@michaelpj michaelpj merged commit d33f5f0 into master Apr 23, 2024
38 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HLS stuck (init then no progress) since multi home support
5 participants