-
Notifications
You must be signed in to change notification settings - Fork 701
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
Split up Distribution.Simple.Setup #8130
Conversation
Some stats about Distribution.Simple.Setup.Config:
|
@mergify rebase |
❌ Base branch update has failedGit reported the following error:
err-code: A4750 |
@TeofilC great work here! I don't think you need to do much more here (e.g. removing instances can go into separate PR). Do you want to finish it? |
Thanks! I've been meaning to finish off this PR. I'll see if I have some time this week. |
be18582
to
25b9277
Compare
I've had a chance to rebase these changes. The diff is quite large but really it's just splitting up this huge module into some bite sized ones |
The main motivation is to improve parallelism of the module graph. This improves compile times as we can benefit more from multiple cores, but also because GHC is superlinear in the size and complexity of source files. Each set of command line options is moved to its own file, and common utilities are moved into .Common. The interface is kept the same and new modules aren't exposed.
7c073e5
to
733b77e
Compare
Great work! I wanna have a closer look some time this week, but I encourage anyone to go ahead and review: on the surface, this is a very nice refactoring! |
Tremendous, thank you! I think, this can be merged as is. We usually ask for a changelog file (see the link in the PR template), but I'm not sure it's strictly needed for a pure refactoring like this. I have one question, though. The commit message says:
It won't be substantial as long as the rest of the codebase imports
I wonder if there are tools around that could assist in such task... |
Good point @ulysses4ever . I'd be happy to do that. The |
Replace imports of Distribution.Simple.Setup with direct imports of the modules that it re-exports. Some imports are left if most re-exported modules are used.
I've pushed a commit that should remove all but 3 of the imports in There's some more imports in |
Makes sense, thanks a lot for the prompt action! Let's hope the second review will come soon. |
@Mikolaj @Kleidukos I noticed you interacted with some of the comments in this PR. Would you be interested in reviewing (the diff looks daunting but it's just moving things around)? Absolutely no worries if not! |
Hi @TeofilC. Changelog updates would be highly appreciated before we merge this. :) |
Thanks for that speedy response @Kleidukos . I've added a changelog entry in b3e8958 |
@TeofilC I'll let you set the "squash+merge me" label then :) |
@Kleidukos I'm not sure if I can set labels. The little cog doesn't show up for me by Labels |
@TeofilC You have been invited as an outside collaborator with Triage access. Could you please reload the page? |
Cheers @Kleidukos works now! |
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!
This PR is inspired by #8074 , which shows that
Distribution.Simple.Setup
is the heaviest module inlib:Cabal
.This splits this module up into some smaller modules. This is quite natural as the file is already divided into sections for different Cabal commands.
The patch is quite rough right now and I need to clean it up quite a bit and update documentation.
Despite that we can already see some positive results on build times.
Before this change the critical path length for
lib:Cabal
was 44s after this change it is 36.3s.Critical path before the change
Critical path after the change
Before the
Distribution.Simple.Setup
file took 15.6s to compile.Compile time for the split up modules sums to 13.4s
This also shows us what specific parts of that module are leading to slowness namely the bits to do with config flags.