Explicitly model more dependencies on cradle startup #463

alexfmpe · 2025-04-25T17:05:56Z

This is a first batch of changes towards explicitly modeling the data dependencies so it's easier to reason about, find saving opportunities and offload cognitive load to the compiler.

Before

$ time cabal run hie-bios -- debug hie-bios.cabal
real	0m0.710s

$ cabal test all
All 43 tests passed (56.80s)

After

$ time cabal run hie-bios -- debug hie-bios.cabal
real	0m0.635s

$ cabal test all
All 43 tests passed (55.25s)

When working on #458 I often had trouble reasoning about and keeping track of the actual flow due to the MonadFix IO used for lazy reading of program versions.
In particular, the first ghc version check triggers a cabal version check, so trying to read the ghc version from cabal path not only doesn't save any time (since we're preparing a ghc call for it), as it will also cause an infinite cycle if we try to read the ghc version lookup in here. The real fun starts when this is triggered by touching something else that's unwittingly involved in the cycle.

soulomoon · 2025-04-26T08:32:00Z

src/HIE/Bios/Cradle.hs

+  deriving (Functor, Foldable, Traversable)
+
+makeVersions :: LogAction IO (WithSeverity Log) -> FilePath -> IO BuildToolVersions
+makeVersions l wdir = mapConcurrently (\v -> v l wdir) $ BuildToolVersions getCabalVersion getStackVersion


why do we use mapConcurrently here?

Welp, forgot to click submit on my own review back when opening the PR.

why do we use mapConcurrently here?

To allow running these in parallel when compiled with an executable using -threaded. See also my other comment on this line.
There's not much difference between sum and max when only one tool is installed as the other will fail near-instantly, and they take basically the same time when both are present.
The downside is that we do an extra version check we wouldn't do with the old lazy approach when all the following are true

-threaded is absent

both tools are installed

stack-only or cabal-only cradle

I do think we can again avoid unneeded calls by first looking at all the cradle types and seeing what we will actually need to load instead of loading all unconditionally but my first priority was cutting the fragile mdo knot without incurring slowdowns so I'm doing baby steps.

That said, stack's version doesn't seem to be used anywhere so I'm not sure why we even read it.
Code compiles after simply removing the field definition and the getStackVersion call so it seems to have no effect besides in the exported API which a comment says is for test's sake.

Huh HLS seems to do it's own separate version checks: https://github.com/haskell/haskell-language-server/blob/cd42bcfdae18b3e377375f495f9739027f3300fc/src/Ide/Version.hs#L62

alexfmpe · 2025-04-25T17:08:40Z

src/HIE/Bios/Cradle.hs

+  deriving (Functor, Foldable, Traversable)
+
+makeVersions :: LogAction IO (WithSeverity Log) -> FilePath -> IO BuildToolVersions
+makeVersions l wdir = mapConcurrently (\v -> v l wdir) $ BuildToolVersions getCabalVersion getStackVersion


These both take around 40ms -+ 10ms for me, so it doesn't seem that either has to wait much for the other, which would slow down stack only or cabal only scenarios.
Theoretically it speeds up the case where both are needed.
That said, stackVersion doesn't seem to be used anywhere?

I think it is safe to just remove it since it is not even used. Easy to add them back if we need them in the future

I feel like it should be re-used by HLS instead of having separate impls.
Maybe it was at some point?

alexfmpe · 2025-04-25T17:09:44Z

src/HIE/Bios/Cradle.hs

    , isCabalMultipleCompSupported
-    , ProgramVersions
+    , BuildToolVersions


Breaking change. Can't seem to avoid it while cutting the knot - at most the old name can be kept.

alexfmpe · 2025-04-25T17:10:37Z

src/HIE/Bios/Cradle.hs

@@ -296,15 +279,15 @@ resolvedCradlesToCradle logger buildCustomCradle root cs = mdo
    notNoneType _ = True


-resolveCradleAction :: Show a => LogAction IO (WithSeverity Log) -> (b -> CradleAction a) -> ResolvedCradles b -> FilePath -> ResolvedCradle b -> CradleAction a
-resolveCradleAction l buildCustomCradle cs root cradle = addLoadStyleLogToCradleAction $
+resolveCradleAction :: Show a => LogAction IO (WithSeverity Log) -> (b -> CradleAction a) -> ResolvedCradles b -> FilePath -> ResolvedCradle b -> IO (CradleAction a)


I think this one can be made pure again, but it needs some other changes first

alexfmpe · 2025-04-25T17:11:15Z

src/HIE/Bios/Cradle.hs

+    cabalPathOutput = case res of
+      CradleSuccess out -> out
+      _ -> Nothing


This is pretty awkward. Maybe callCabalPathForCompilerPath should be IO instead of CradleResultT ?

alexfmpe · 2025-04-26T13:51:02Z

src/HIE/Bios/Cradle.hs

+  deriving (Functor, Foldable, Traversable)
+
+makeVersions :: LogAction IO (WithSeverity Log) -> FilePath -> IO BuildToolVersions
+makeVersions l wdir = mapConcurrently (\v -> v l wdir) $ BuildToolVersions getCabalVersion getStackVersion


Welp, forgot to click submit on my own review back when opening the PR.

why do we use mapConcurrently here?

To allow running these in parallel when compiled with an executable using -threaded. See also my other comment on this line.
There's not much difference between sum and max when only one tool is installed as the other will fail near-instantly, and they take basically the same time when both are present.
The downside is that we do an extra version check we wouldn't do with the old lazy approach when all the following are true

-threaded is absent

both tools are installed

stack-only or cabal-only cradle

I do think we can again avoid unneeded calls by first looking at all the cradle types and seeing what we will actually need to load instead of loading all unconditionally but my first priority was cutting the fragile mdo knot without incurring slowdowns so I'm doing baby steps.

That said, stack's version doesn't seem to be used anywhere so I'm not sure why we even read it.
Code compiles after simply removing the field definition and the getStackVersion call so it seems to have no effect besides in the exported API which a comment says is for test's sake.

alexfmpe · 2025-04-26T14:15:33Z

src/HIE/Bios/Cradle.hs

+  deriving (Functor, Foldable, Traversable)
+
+makeVersions :: LogAction IO (WithSeverity Log) -> FilePath -> IO BuildToolVersions
+makeVersions l wdir = mapConcurrently (\v -> v l wdir) $ BuildToolVersions getCabalVersion getStackVersion


Huh HLS seems to do it's own separate version checks: https://github.com/haskell/haskell-language-server/blob/cd42bcfdae18b3e377375f495f9739027f3300fc/src/Ide/Version.hs#L62

… versions up front

alexfmpe · 2025-06-04T15:52:40Z

src/HIE/Bios/Cradle.hs

      -- We trust the assertion for a bios program, as we have no way of
      -- checking its version
      | LoadWithContext _ <- loadStyle ->
-          if ghc >= makeVersion [9,4]
+          if isCabalMultipleCompSupported Nothing (Just ghc)


We can just do pure $ bool LoadFile loadStyle $ isCabalMultipleCompSupported Nothing ghc_version for the whole thing if we don't care about the specific log below.
Alternatively, if we always want it on every check, we can also have a util that does that consistently.

alexfmpe force-pushed the cabal-path-correctness branch from 00c31c8 to a7a9fed Compare April 26, 2025 00:17

soulomoon reviewed Apr 26, 2025

View reviewed changes

alexfmpe commented Apr 26, 2025

View reviewed changes

Only call 'cabal path' once when resolving a cradle

4b8fe70

alexfmpe force-pushed the cabal-path-correctness branch from a7a9fed to cc3ac98 Compare June 4, 2025 14:47

alexfmpe added 2 commits June 4, 2025 16:28

Avoid fixed-point implicit dependencies by eagerly loading build tool…

d7f1bc3

… versions up front

Parse ghc version from cabal path

3025d2d

alexfmpe force-pushed the cabal-path-correctness branch from cc3ac98 to 3025d2d Compare June 4, 2025 15:28

DRY multi-component support checks

0485f71

alexfmpe commented Jun 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Explicitly model more dependencies on cradle startup #463

Explicitly model more dependencies on cradle startup #463

Uh oh!

alexfmpe commented Apr 25, 2025 •

edited

Loading

Uh oh!

soulomoon Apr 26, 2025

Uh oh!

alexfmpe Apr 26, 2025

Uh oh!

alexfmpe Apr 26, 2025

Uh oh!

alexfmpe Apr 25, 2025

Uh oh!

soulomoon Apr 27, 2025

Uh oh!

alexfmpe Apr 27, 2025

Uh oh!

alexfmpe Apr 25, 2025

Uh oh!

alexfmpe Apr 25, 2025

Uh oh!

alexfmpe Apr 25, 2025

Uh oh!

alexfmpe Apr 26, 2025

Uh oh!

alexfmpe Apr 26, 2025

Uh oh!

alexfmpe Jun 4, 2025

Uh oh!

Uh oh!

Explicitly model more dependencies on cradle startup #463

Are you sure you want to change the base?

Explicitly model more dependencies on cradle startup #463

Uh oh!

Conversation

alexfmpe commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexfmpe commented Apr 25, 2025 •

edited

Loading