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

haskellPackages: build with RTS -A64M options #317251

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

wolfgangwalther
Copy link
Contributor

While working on #317181 I realized that the +RTS -A64M -RTS options in parallelBuildingFlags were never passed to GHC, because the arguments are not passed quoted. This means those arguments are actually passed to Setup.hs. This can be verified by removing -rtsopts from setupCompileFlags - after this change the configure command will throw an error, because it doesn't understand the +RTS ... argument anymore. Once you pass it properly, the error is gone again and GHC receives the arguments.

Passing the RTS flags to Setup.hs (i.e. cabal) - makes little to zero sense. The same is true for the entire parallelBuildingFlags being passed to setupCompileFlags: If there was any custom Setup.hs script in any package anywhere that benefits from heavy parallelization like this... there would be something wrong with that script! ;)

Those flags were introduced in #86948. The related twitch live stream uses the build of git-annex as a measurement. I get the following numbers when building git-annex with doCheck = false;:

  • current master: 1:40 wall clock / 340s user
  • without -A64M: 1:40 wall clock / 340s user
  • with this fix: 1:13 wall clock / 280s user

This 27% improvement is essentially the result from that live stream mentioned above. Let's use that, shall we?

Things done

  • Built on platform(s)
    • x86_64-linux: haskell.lib.overrideCabal haskellPackages.git-annex { doCheck = false; }
      [...]
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

Otherwise this makes sense to me. -A64M is beneficial in multi-threaded situations. As far as I'm aware, Cabal is single-threaded for our usecase while ghc (even with our current escaping mess) receives a -j flag.

pkgs/development/haskell-modules/generic-builder.nix Outdated Show resolved Hide resolved
@sternenseemann
Copy link
Member

My testing with lens seems to confirm what you are saying (though I got very noisy data).

Those flags were not actually passed to GHC before, but to Setup.hs.

They were introduced in NixOS#86948. The related twitch live stream uses the
build of git-annex as a measurement. I get the following numbers when
building git-annex with doCheck = false:

 - for current master: 1:40 wall clock / 340s user
 - without any -A64M argument: 1:40 wall clock / 340s user
 - with this fix: 1:13 wall clock / 280s user

The idea was good, but the settings were never active.

More testing revealed that this seems to work on darwin just as well, so
we're removing the isLinux condition, too.
@sternenseemann sternenseemann merged commit e160c2a into NixOS:haskell-updates Jun 10, 2024
20 checks passed
@sternenseemann
Copy link
Member

We should consider backporting this and #317181 after testing for regressions (#318363)!

@sternenseemann sternenseemann added the 9.needs: port to stable A PR needs a backport to the stable release. label Jun 10, 2024
@wolfgangwalther wolfgangwalther deleted the rtsopts branch June 10, 2024 15:16
@sternenseemann sternenseemann added backport staging-24.05 Backport PR automatically and removed 9.needs: port to stable A PR needs a backport to the stable release. labels Jun 20, 2024
Copy link
Contributor

Successfully created backport PR for staging-24.05:

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

Successfully merging this pull request may close these issues.

2 participants