-
Notifications
You must be signed in to change notification settings - Fork 691
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
wip: feature: pass ParStrat to build extra sources #9872
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very welcome improvement.
Great work both here and in GHC @edmundnoble.
Regarding the checklist:
- Writing this down in the changelog is nice to advertise the performance improvements gained here
- No documentation needs updating I think
- I think you could write an automated test.
To test this automatically I suggest you write a simple cabal.test.hs
PackageTest
for a package with two source files which calls cabal with -v2
and -jsem
and checks that the extra-sources ghc invocation line contains both -jsem
and the two source files at once. Take a look at other cabal.test.hs
files (they're pretty easy to write).
forM_ allOpts $ \opts -> do | ||
files <- fmap concat $ forM sourceFiles $ \sourceFile -> do | ||
needsRecomp <- checkNeedsRecompilation sourceFile opts | ||
return [sourceFile | needsRecomp] | ||
when (not (null files)) $ | ||
runGhcProg opts { ghcOptInputFiles = toNubListR files } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that is much better than doing one ghc invocation for each extra source. That alone should already be performance significant when compiling multiple C sources, let alone eventually building all those in parallel.
One limitation of this approach is that if you have way too many C sources within many nested folders you may run into command argument size limitations (e.g. see https://www.in-ulm.de/~mascheck/various/argmax/). Though we aren't that careful and may e.g. already reach this limitation when linking together all Haskell modules using the path to every .o file (see GHC.Build.Link's Perhaps we should use a response file if the list of sources is too large, but it is a bit hard to tell because it also depends on the length of the path to the source file. |
What has been the testing strategy for this patch? Are we very sure that removing the order dependence doesn't break some packages? It would also be better to guard passing |
Pass
ParStrat
to GHC when we build extra sources, and batch up file paths into GHC invocations, so that GHC can build them in parallel.This doesn't actually make GHC build these sources in parallel right now, because GHC in one-shot mode doesn't even listen to
-j
, though it doesn't cause an error or anything. But it hopefully will soon! https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12388An alternative would be to make GHC's
--make
understand that it has to build foreign sources after Haskell modules. I didn't investigate that possibility too thoroughly.QA Notes
GHC invocations when building projects with foreign sources should have a) all of those sources in a single GHC invocation and b)
-jsem
if--semaphore
was passed to cabal.Template Α: This PR modifies
cabal
behaviourInclude the following checklist in your PR: