Improve the performance of GetModIfaceFromDisk in large repos and delete GetDependencies

[pepeiborra]

This changes GhcSessionDeps to reuse the GhcSessionDeps of the dependencies, and then GetModIfaceFromDisk to use GhcSessionDeps instead of calling ghcSessionDepsDefinition.

There are three benefits:

1. GetModIfaceFromDisk and GhcSessionDeps no longer depend on the transitive module summaries. This means fewer edges in the build graph = smaller build graph = faster builds
2. Avoid duplicate computations in setting up the GHC session with the dependencies of the module. Previously the total work done was O(NlogN) in the number of transitive dependencies, now it is O(N).
3. Increased sharing of HPT and FinderCache.

Ideally we should also share the module graphs, but the datatype is abstract, doesn't have a Monoid instance, and cannot be coerced to something that has. We will need to add the Monoid instance in GHC first.

On the Sigma repo:

• the startup metric goes down by ~34%.
• The edit metric also goes down by 15%.
• Max residency is down by 30% in the edit benchmark.

Interestingly, the memory usage goes down despite caching more!

Fixes #2304 (in that it improves the complexity as much as I can think of)

[pepeiborra]

One surprising and unintended consequence of this is that GetDependencies is not getting pulled by anything anymore, and therefore neither is ReportImportCycles. This is unfortunate for two reasons:

1. Import cycles no longer get diagnostics
2. Import cycles will lead to build graph cycles, and the IDE will stop producing diagnostics (Shake) or even blow up (hls-graph)

I suppose we'll need to introduce an artificial dependency on GetDependencies, but doing that will reintroduce O(imports^2) edges in the build graph which is damn expensive.

A better option would be to handle cycles in the build system (hls-graph), and make them visible in build rules somehow so that we can convert them in diagnostics. Any thoughts @ndmitchell ? You have cycle detection in Shake, would it work for his-graph?

[pepeiborra]

I suppose we'll need to introduce an artificial dependency on GetDependencies, but doing that will reintroduce O(imports^2) edges in the build graph which is damn expensive.

I have done something slightly different that hopefully preservers the current semantics - a synthetic dependency on ReportImportCycles in GhcSessionDeps for all the direct imports, whereas previously it was being done for all the transitive imports. Direct imports should be enough since GhcSessionDeps is recursive, and therefore it will hit all the transitive imports eventually. Direct imports also means that the horrible O(imports^2) cost is improved in most cases, although it's probably still the same in the worst case.

I have also deleted GetDependencies since now it's not being used anywhere.

[ndmitchell]

You have cycle detection in Shake, would it work for his-graph?

There are two cycle detections in Shake. One uses a stack of basically where you have come from, and produces moderately good error messages. If you enhanced that with the stack in terms of import definitions you would get nice user actionable error messages.

The second cycle detector is the worse one. When A and B are mutually depended upon, and both start simultaneously, they both notice each other is in progress, and then everything halts. Shake gives a really poor error here, but does spot the cycle because it has things to be done, and can't make progress. To get a good error message would require fairly complex graph algorithms. There are some, but they are probably a lot more work than doing a dependency scan in the rules.

[pepeiborra]

@jneira any idea of why the func-test testsuite is timing out on Windows w/ ghc 9.0 after >4h? It normally takes 3m

[pepeiborra]

@wz1000 are you able to review this core change?

[wz1000]

Looks like a good improvement.

[wz1000]

This can easily break with new GHC releases. Can we guard this using CPP for GHC versions where this is known to be ok? It can be a compile error for as of yet unreleased GHC versions.

[pepeiborra]

I think we will find out if this breaks with a new GHC release, not sure what's the benefit of preemptive CPP

[pepeiborra]

Moreover, the missing instances will be added hopefully soon - https://gitlab.haskell.org/ghc/ghc/-/merge_requests/6935

[jneira]

@jneira any idea of why the func-test testsuite is timing out on Windows w/ ghc 9.0 after >4h? It normally takes 3m

I got to enable all tests but progress ones in #2296 but by pure brute force, after that it has not timed out. I have to investigate why progress tests continue causing hangs although i suspect about eval one.

haskell-language-server/test/functional/Main.hs

Line 37 in 6fb0dc0

, ignoreInEnv [HostOS Windows, GhcVer GHC90] "Tests gets stuck in ci" $ Progress.tests

[pepeiborra]

@jneira any idea of why the func-test testsuite is timing out on Windows w/ ghc 9.0 after >4h? It normally takes 3m

I got to enable all tests but progress ones in #2296 but by pure brute force, after that it has not timed out. I have to investigate why progress tests continue causing hangs although i suspect about eval one.

haskell-language-server/test/functional/Main.hs

Line 37 in 6fb0dc0

, ignoreInEnv [HostOS Windows, GhcVer GHC90] "Tests gets stuck in ci" $ Progress.tests

I see a timeout as well in the CI results for #2296

[jneira]

Yeah, i tried all combinations of ignore specific test groups several times and several attempts ended with hangs for windows and 9.0.1 until https://github.com/haskell/haskell-language-server/runs/4079556307?check_suite_focus=true ignoring only progress tests.

After that i've not seen another run get stuck in windows and 9.0.1 in any branch (i've just did a search in subsequent runs to find it and i did not see anyone but maybe i missed it).

[jneira]

Oh you are referring to https://github.com/haskell/haskell-language-server/runs/4088340871?check_suite_focus=true?
Maybe the hang is less probable now but it continue happening... 🤔
In my repo the same commits are not causing the hangs and of course they are not reproduced locally

[jneira]

Lets see how is going the actual workflow run, will try to disable other test groups if the hang is reproduced (or afterwards if it happens again in another branch)

[jneira]

The func-test suite did not hang this time: https://github.com/haskell/haskell-language-server/runs/4150189056?check_suite_focus=true
With a little bit of luck the rest of jobs will pass 🤞

[pepeiborra]

I'm not going to merge this yet since the benchmarks show a perf regression after introducing the synthetic dependency on ReportImportCycles.

[pepeiborra]

The func-test suite did not hang this time: https://github.com/haskell/haskell-language-server/runs/4150189056?check_suite_focus=true
With a little bit of luck the rest of jobs will pass 🤞

While it didn't time out this time around, it timed out for 4/5 previous attempts, so I strongly think that we should disable it (or the offending tests)

[pepeiborra]

With the new build graph statistics outputs in the benchmark suite (#2343) it's easier to see what's causing the regression:

version    name               success   samples   startup              setup   userTime             delayedTime             totalTime            buildRulesBuilt   buildRulesChanged   buildRulesVisited   buildRulesTotal   buildEdges   maxResidency   allocatedBytes
upstream   edit               True      50        2.195506             0.0     10.804728000000003   1.7481000000000003e-2   10.829789            17                13                  1499                6054              138396       403MB          29825MB
nocycles   edit               True      50        2.439721             0.0     10.207598            9.900999999999997e-2    10.314124000000001   18                14                  1463                5584              36562        366MB          28741MB
HEAD       edit               True      50        1.9325120000000002   0.0     13.451666999999997   1.7155999999999998e-2   13.475761            183               97                  1629                6040              117117       390MB          38599MB

Compared to upstream, this PR HEAD increases the number of rules built on edit by a factor of 10.

It also shows the cost (3X build graph edges) of the import cycle tracking rules.

[pepeiborra]

Perf regression fixed, benchmarks look fine:

version    name                             success   samples   startup              setup                   userTime             delayedTime             totalTime            buildRulesBuilt   buildRulesChanged   buildRulesVisited   buildRulesTotal   buildEdges   maxResidency   allocatedBytes
upstream   edit                             True      50        2.740945656          0.0                     23.840065159999998   5.403511798             29.251490501000003   18                14                  1500                6054              138396       347MB          28401MB
HEAD       edit                             True      50        2.678469225          0.0                     29.585207137000012   2.8267619e-2            29.620908206000003   17                13                  1628                6040              117117       349MB          28736MB

[wz1000]

What was the fix?