Add `--semaphore` flag to enable interaction with GHC Job Server… #8557

mpickering · 2022-10-28T11:14:10Z

… (-jsem)

This is a WIP patch which allows testing GHC jobserver (-jsem) using cabal. When --semaphore is passed to cabal-install then cabal will create a semaphore with the number of capabilities suggested by -j and then use this semaphore to mediate parallelism between GHC processes.

Cabal/src/Distribution/Simple/Build.hs

Cabal/src/Distribution/Simple/GHC.hs

Cabal/src/Distribution/Types/ParStrat.hs

cabal-install/src/Distribution/Client/JobControl.hs

sheaf · 2023-03-16T12:38:30Z

cabal-install/src/Distribution/Client/JobControl.hs

+          res <- try job `finally` releaseSemaphore sem 1
+          atomically $ writeTChan outqVar res
+        -- Wait for spawned process to take some capabilities if it wants some
+        threadDelay 1_000_000


Is there any sense in making this configurable? Have you experimented with different values?

I don't think so.. I didn't try any different values. I think just something non-zero is good and not so small to prevent thrashing.

Cabal/src/Distribution/Simple/Build.hs

doc/cabal-project.rst

mpickering · 2023-03-21T11:57:30Z

Updated based on @sheaf s comments now

Made the semaphore name fresh, so that you can run multiple cabal processes at once.
Parametrised the ParStrat type so it can be used by cabal-install and Cabal
Fixed the logic taking/returning semaphores to correctly handle async exceptions (thanks @sheaf for discussing this with me)

mpickering · 2023-03-21T13:07:40Z

Testing this feature will have to wait because there isn't a released compiler yet which supports -jsem.

changelog.d/pr-8557

changelog.d/pr-8557-2

changelog.d/pr-8557

changelog.d/pr-8557-2

fgaz · 2023-03-23T07:23:31Z

This will close #976, right?

fgaz · 2023-03-23T07:33:27Z

cabal-install/src/Distribution/Client/Setup.hs

+      , option [] ["semaphore"]
+          "Use a semaphore so GHC can compile components in parallel"
+          installUseSemaphore (\v flags -> flags { installUseSemaphore = v })
+          trueArg


maybe this should be a boolOpt, so it can be enabled or disabled in the global or project config (I assume we'll enable this by default after the next release if not in this pr)

I changed this to boolOpt.

I am not sure how to go about enabling it by default because you would also need a very new version of GHC, at the moment if it's enabled and you use an older version of GHC then you get an error (rather than falling back to old behaviour).

I implemented it now so that if you use an older compiler and pass --sempahore then you get a warning about falling back to old behaviour but things still work. That way it can be enabled globally without too much hassle.

I actually changed the option to a yesNoOpt which seemed to fit in better.. and also hopefully fixes some failing roundtrip tests I couldn't work out how to fix.

mpickering · 2023-03-23T09:43:04Z

This will close #976, right?

It seems related to #976... but there are lots of ideas there and the idea of using one shot mode could still be implemented and give better parallelism results.

mpickering · 2023-03-23T11:35:38Z

As a data point, using this feature to compile exe:cabal in the cabal repo halved the build time.

mpickering · 2023-03-23T15:54:44Z

CI is passing now :)

Kleidukos · 2023-07-12T10:35:19Z

@sheaf @fgaz ok I believe we're good now but I'd highly appreciate a last review from you so that we can conclude this saga. :)

Cabal/src/Distribution/Simple/Build.hs

Cabal/src/Distribution/Simple/Compiler.hs

cabal-install/src/Distribution/Client/CmdHaddockProject.hs

cabal-install/src/Distribution/Client/Config.hs

cabal-install/src/Distribution/Client/ProjectBuilding.hs

cabal-testsuite/PackageTests/NewBuild/Semaphore/cabal.test.hs

changelog.d/pr-8557-2

* The top-level user interface to enable the use of the semaphore is the `--semaphore` flag. If you pass `-j4 --semaphore` then cabal will create a semaphore with 4 slots which is passed to ghc using the `-jsem` option so that all GHC processes coordinate to use 4 capabilities. - The semaphore logic is provided by a new package `semaphore-compat` which provides a cross-platform abstraction for semaphores. * The low level `./Setup.hs build` interface accepts the `--semaphore <SEMAPHORE>` option, which can be used to directly pass the semaphore you require to the Setup script.

ffaf1

Good as far as I can see!

Kleidukos · 2023-07-14T12:22:24Z

@Mergifyio backport 3.10

mergify · 2023-07-14T12:22:27Z

backport 3.10

🟠 Waiting for conditions to match

merged [:pushpin: backport requirement]

ulysses4ever · 2023-07-18T02:49:48Z

GitHub needed a rebase and I pushed the button. Results are not hopeful:

one lint failure
one actual failure because of a change to the options interface

bgamari · 2023-07-18T17:58:30Z

I have rebased this and opened #9139 proposing merge.

andreabedini · 2023-07-19T05:17:56Z

I understand this was subsumed by #9139 which is now merged. Please reopen if I am mistaken.

mpickering force-pushed the wip/jsem branch from e40f816 to d4105b1 Compare December 18, 2022 13:28

ulysses4ever marked this pull request as draft February 7, 2023 03:06

mpickering force-pushed the wip/jsem branch 2 times, most recently from 3a6312e to 5563a57 Compare March 16, 2023 12:11

sheaf reviewed Mar 16, 2023

View reviewed changes

mpickering force-pushed the wip/jsem branch from 7fe2f19 to dbe9ff0 Compare March 21, 2023 11:55

mpickering force-pushed the wip/jsem branch from dbe9ff0 to dc93380 Compare March 21, 2023 13:04

mpickering changed the title ~~WIP: Add --semaphore flag to enable interaction with GHC Job Server…~~ Add --semaphore flag to enable interaction with GHC Job Server… Mar 21, 2023

mpickering force-pushed the wip/jsem branch from dc93380 to cab1afc Compare March 21, 2023 17:39

mpickering marked this pull request as ready for review March 22, 2023 11:40

mpickering force-pushed the wip/jsem branch 2 times, most recently from 95c8d6a to 660d45c Compare March 22, 2023 11:52

sheaf reviewed Mar 22, 2023

View reviewed changes

changelog.d/pr-8557 Outdated Show resolved Hide resolved

changelog.d/pr-8557 Outdated Show resolved Hide resolved

changelog.d/pr-8557 Outdated Show resolved Hide resolved

changelog.d/pr-8557-2 Outdated Show resolved Hide resolved

mpickering force-pushed the wip/jsem branch 2 times, most recently from 5c4fd97 to 1d74397 Compare March 22, 2023 13:57

sheaf reviewed Mar 22, 2023

View reviewed changes

changelog.d/pr-8557 Outdated Show resolved Hide resolved

changelog.d/pr-8557-2 Outdated Show resolved Hide resolved

fgaz reviewed Mar 23, 2023

View reviewed changes

mpickering force-pushed the wip/jsem branch 4 times, most recently from 8cb82fb to 3303449 Compare March 23, 2023 11:32

mpickering force-pushed the wip/jsem branch 3 times, most recently from a7331a8 to 5c6c4ce Compare March 23, 2023 15:11

Kleidukos force-pushed the wip/jsem branch 2 times, most recently from 959c4a2 to d63ae09 Compare July 11, 2023 21:12

Kleidukos removed attention: needs-help Help wanted with this issue/PR attention: needs-rebase labels Jul 13, 2023

ffaf1 reviewed Jul 14, 2023

View reviewed changes

Kleidukos reviewed Jul 14, 2023

View reviewed changes

changelog.d/pr-8557-2 Show resolved Hide resolved

mpickering and others added 9 commits July 14, 2023 11:11

Review comments

dfdda9a

Add test

4374f71

Rebase and format

1efe899

fix imports

a508387

Fix rebase conflicts

7619cf3

Fix rebase conflict for options

1e50d75

restore multireploptions

e9f06f0

Address reviews

25e00c3

Kleidukos force-pushed the wip/jsem branch 2 times, most recently from 7fbfb0f to 25e00c3 Compare July 14, 2023 09:17

ffaf1 approved these changes Jul 14, 2023

View reviewed changes

Kleidukos added the squash+merge me Tell Mergify Bot to squash-merge label Jul 14, 2023

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jul 16, 2023

mergify bot and others added 2 commits July 16, 2023 12:24

Merge branch 'master' into wip/jsem

0cea6fe

Merge branch 'master' into wip/jsem

5fdf1b9

bgamari mentioned this pull request Jul 18, 2023

Add support for using GHC's -jsem option #9139

Merged

andreabedini closed this Jul 19, 2023

Add --semaphore flag to enable interaction with GHC Job Server… #8557

Add --semaphore flag to enable interaction with GHC Job Server… #8557

Uh oh!

Conversation

mpickering commented Oct 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sheaf Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

mpickering Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mpickering commented Mar 21, 2023

Uh oh!

mpickering commented Mar 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fgaz commented Mar 23, 2023

Uh oh!

fgaz Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpickering Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

mpickering Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

mpickering Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

mpickering commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpickering commented Mar 23, 2023

Uh oh!

mpickering commented Mar 23, 2023

Uh oh!

Kleidukos commented Jul 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ffaf1 left a comment

Choose a reason for hiding this comment

Uh oh!

Kleidukos commented Jul 14, 2023

Uh oh!

mergify bot commented Jul 14, 2023

🟠 Waiting for conditions to match

Uh oh!

ulysses4ever commented Jul 18, 2023

Uh oh!

bgamari commented Jul 18, 2023

Uh oh!

andreabedini commented Jul 19, 2023

Uh oh!

Add `--semaphore` flag to enable interaction with GHC Job Server… #8557

Add `--semaphore` flag to enable interaction with GHC Job Server… #8557

fgaz Mar 23, 2023 •

edited

Loading

mpickering commented Mar 23, 2023 •

edited

Loading