Share ModuleGraphs for all files

[wz1000]

Remove GetDependencyInformation in favour of GetModuleGraph

Computing and storing GetDependencyInformation for each file essentially individually means
that we perform downsweep on each file individually, wasting a lot of work and using an excessive
amount of memory to store all these duplicated graphs individually.

However, we already have the GetModuleGraph rule, which we need to compute before compiling
files any way due to being depended on by NeedsCompilation, which needs to know if any reverse
dependencies of the module we are compiling requires TH, which meant that each file already depends on
the results of downsweep for the whole project.

Instead, we can compute the whole graph once when we execute the GetModuleGraph rule and even use this inside HscEnv.hsc_mod_graph to avoid reconstructing the ModuleGraph on each invocation of GhcSessionDeps.

There may be concerns about excessive build churn due to any change to the result of GetModuleGraph
invalidating the result of GhcSessionDeps too often, but note that this only happens when something
in the header of a module changes, and this could be solved easily be re-introducing
a version of GetDependencyInformation with early cutoff that essentially returns the result of GetModuleGraph
but includes the hash of only the ModSummarys in the downward dependency closure of the file.

[pepeiborra]

How do you deal with the cycles issue? If there are cycles in the module graph, then you end up with cycles in the build graph, and hls-graph cannot handle them.

This has been discussed before: #1359, #2323 (comment)

[wz1000]

How do you deal with the cycles issue? If there are cycles in the module graph, then you end up with cycles in the build graph, and hls-graph cannot handle them.

This has been discussed before: #1359, #2323 (comment)

There is no rule cycle because we now construct the ModuleGraph a single time using all the known targets. A GetModuleGraph rule doesn't depend on other GetModuleGraph rules (infact it is a defineNoFile rule, so there is only one instance). The rawDependencyInformation function already dealt with cycles by maintaining state for modules which have already been visited.

[pepeiborra]

There may be concerns about excessive build churn due to any change to the result of GetModuleGraph
invalidating the result of GhcSessionDeps too often, but note that this only happens when something
in the header of a module changes,

I have concerns about this. It's not just build churn, but the fact that to load a module first I need to compute the module graph for the entire codebase. How is this going to affect init times for large codebases with >10.000 modules?

this could be solved easily be re-introducing
a version of GetDependencyInformation with early cutoff that essentially returns the result of GetModuleGraph
but includes the hash of only the ModSummarys in the downward dependency closure of the file.

By all means, include this in the PR. But it doesn't solve the startup cost issue.

[wz1000]

@pepeiborra I've replace checkForImportCycles with a fullModuleGraph option that tries to restore the older behaviour.

[pepeiborra]

@pepeiborra I've replace checkForImportCycles with a fullModuleGraph option that tries to restore the older behaviour.

where is the option? I cannot see it

[wz1000]

@pepeiborra I've replace checkForImportCycles with a fullModuleGraph option that tries to restore the older behaviour.

where is the option? I cannot see it

Check now, the push was still going when I made the comment.

[pepeiborra]

@pepeiborra I've replace checkForImportCycles with a fullModuleGraph option that tries to restore the older behaviour.

where is the option? I cannot see it

Check now, the push was still going when I made the comment.

The option looks good, but I kind of want to see a benchmark in a large code base. Both with the full module graph and without it.

Do you still want to get this merged before the next release? If it cannot wait, then go ahead and merge it

[wz1000]

This needs #3391 to work because the eval plugin currently relies on the driver (it calls load) which inspects ms_hs_date which we unset for GetModSummaryWithoutTimestamps. It is not desirable to use GetModSummary for the module graph as then any change to any file will invalidate the module graph (which all files depend on), rather than just changes to the module header.

We shouldn't be using the GHC driver anyway as we want to perform loading and recompilation checking using our own logic. The rework of the eval plugin fixes this.

I will land them both after the release.

[pepeiborra]

@wz1000 I benchmarked this change in Bigcorp codebase and the results were not good. 6X longer startup times with full module graph and 5X without full module graph. This is compared to my O(imports) GetDependencies hack, which for reference is simply:

getDependencies :: NormalizedFilePath -> Action TransitiveDependencies
getDependencies f = do
  imports <- use_ GetLocatedImports f
  let direct = [artifactFilePath | (_, Just ArtifactsLocation {..}) <- imports]
  transitive <- foldMap transitiveModuleDeps <$> uses_ GetDependencies direct
  let !final = HashSet.toList $ HashSet.fromList $ direct <> transitive
  return $ TransitiveDependencies final

Full results:

BEFORE

name         | success | samples | startup | setup | userTime | delayedTime | totalTime
------------ | ------- | ------- | ------- | ----- | -------- | ----------- | ---------
edit         | True    | 10      | 2m10s   | 0.00s | 13.82s   | 0.00s       | 13.83s
edit imports | True    | 10      | 1m49s   | 0.00s | 0.27s    | 0.00s       | 0.27s
/data/users/pepeiborra/fbsource/fbcode/buck-out/opt/gen/sigma/ide/sigma-ide --lsp --test --cwd /home/pepeiborra/si_sigma +RTS -A32M -I0 -s/tmp/sigma-ide-benchmark-large.gcStats
 355,733,992,672 bytes allocated in the heap
  52,305,367,504 bytes copied during GC
  13,424,971,888 bytes maximum residency (10 sample(s))
     552,684,264 bytes maximum slop
           12803 MB total memory in use (0 MB lost due to fragmentation)

                                     Tot time (elapsed)  Avg pause  Max pause
  Gen  0       413 colls,   412 par   563.854s  14.237s     0.0345s    0.5817s
  Gen  1        10 colls,     8 par   290.148s  18.952s     1.8952s    10.9664s

  Parallel GC work balance: 89.61% (serial 0%, perfect 100%)

  TASKS: 198 (7 bound, 168 peak workers (191 total), using -N40)

  SPARKS: 0 (0 converted, 0 overflowed, 0 dud, 0 GC'd, 0 fizzled)

  INIT    time    0.001s  (  0.001s elapsed)
  MUT     time  378.871s  ( 89.268s elapsed)
  GC      time  854.002s  ( 33.189s elapsed)
  EXIT    time    0.043s  (  0.006s elapsed)
  Total   time  1232.917s  (122.464s elapsed)

  Alloc rate    938,932,822 bytes per MUT second

  Productivity  30.7% of total user, 72.9% of total elapsed

fullModulegraph=true

name | success | samples | startup | setup | userTime | delayedTime | totalTime
---- | ------- | ------- | ------- | ----- | -------- | ----------- | ---------
edit | True    | 10      | 12m42s  | 0.00s | 13.31s   | 0.00s       | 13.32s
/data/users/pepeiborra/fbsource/fbcode/buck-out/opt/gen/sigma/ide/sigma-ide --lsp --test --cwd /home/pepeiborra/si_sigma +RTS -A32M -I0 -s/tmp/sigma-ide-benchmark-large.gcStats
3,707,510,882,688 bytes allocated in the heap
  96,192,538,160 bytes copied during GC
  19,762,467,872 bytes maximum residency (14 sample(s))
     604,503,008 bytes maximum slop
           18846 MB total memory in use (0 MB lost due to fragmentation)

                                     Tot time (elapsed)  Avg pause  Max pause
  Gen  0      4080 colls,  4079 par   1592.894s  39.927s     0.0098s    0.8547s
  Gen  1        14 colls,    12 par   589.262s  45.278s     3.2342s    29.5445s

  Parallel GC work balance: 81.60% (serial 0%, perfect 100%)

  TASKS: 284 (7 bound, 270 peak workers (277 total), using -N40)

  SPARKS: 0 (0 converted, 0 overflowed, 0 dud, 0 GC'd, 0 fizzled)

  INIT    time    0.001s  (  0.001s elapsed)
  MUT     time  3939.385s  (722.147s elapsed)
  GC      time  2182.156s  ( 85.205s elapsed)
  EXIT    time    0.089s  (  0.008s elapsed)
  Total   time  6121.632s  (807.361s elapsed)

  Alloc rate    941,139,498 bytes per MUT second

  Productivity  64.4% of total user, 89.4% of total elapsed

fullModulegraph=false

name | success | samples | startup | setup | userTime | delayedTime | totalTime
---- | ------- | ------- | ------- | ----- | -------- | ----------- | ---------
edit | True    | 10      | 9m46s   | 0.00s | 12.12s   | 0.00s       | 12.12s   
/data/users/pepeiborra/fbsource/fbcode/buck-out/opt/gen/sigma/ide/sigma-ide --lsp --test --cwd /home/pepeiborra/si_sigma +RTS -A32M -I0 -s/tmp/sigma-ide-benchmark-large.gcStats 
3,536,421,762,320 bytes allocated in the heap
  86,187,539,816 bytes copied during GC
  17,900,403,312 bytes maximum residency (15 sample(s))
     547,153,296 bytes maximum slop
           17071 MB total memory in use (0 MB lost due to fragmentation)

                                     Tot time (elapsed)  Avg pause  Max pause
  Gen  0      3636 colls,  3635 par   1222.932s  30.644s     0.0084s    0.4791s
  Gen  1        15 colls,    13 par   549.295s  34.563s     2.3042s    19.7335s

  Parallel GC work balance: 84.83% (serial 0%, perfect 100%)

  TASKS: 268 (7 bound, 255 peak workers (261 total), using -N40)

  SPARKS: 0 (0 converted, 0 overflowed, 0 dud, 0 GC'd, 0 fizzled)

  INIT    time    0.001s  (  0.001s elapsed)
  MUT     time  3415.233s  (554.057s elapsed)
  GC      time  1772.227s  ( 65.207s elapsed)
  EXIT    time    0.091s  (  0.016s elapsed)
  Total   time  5187.552s  (619.282s elapsed)

  Alloc rate    1,035,484,716 bytes per MUT second

  Productivity  65.8% of total user, 89.5% of total elapsed

[wz1000]

@pepeiborra do you mean your hack is separate from what lives in HLS master as of now? Is there any regression in fullModulegraph=false compared to current HLS master? I don't think there should be, but maybe I made a mistake.

I think we could use something like your hack if we can augment hls-graph cycle detection to produce recoverable errors.

[pepeiborra]

@pepeiborra do you mean your hack is separate from what lives in HLS master as of now?

Actually ignore the hack, I was confused, the BEFORE results are not using the hack.

Reason: we are still on ghcide 1.4, while upgrading to ghcide 1.8 I deleted the hack since GetDependencies no longer exists. Startup times with ghcide 1.4 + hack were around 1m20s, on vanilla 1.8 it's 2m10s.

Is there any regression in fullModulegraph=false compared to current HLS master? I don't think there should be, but maybe I made a mistake.

Yes, the regression is 2m10s -> 9m46s for startup. More specifically, by startup I mean loading the first module, which is handpicked to have a very large transitive module graph.

I think we could use something like your hack if we can augment hls-graph cycle detection to produce recoverable errors.

I don't think this can be done cleanly.

[pepeiborra]

With the strictness fix, the regression is less dramatic: 50% longer to startup and 32% more total memory in use (17G of instead of 12.8G). Not quite ready yet, but less bad.

name | success | samples | startup | setup | userTime | delayedTime | totalTime
---- | ------- | ------- | ------- | ----- | -------- | ----------- | ---------
edit | True    | 10      | 3m16s   | 0.00s | 12.04s   | 0.00s       | 12.04s   
/data/users/pepeiborra/fbsource/fbcode/buck-out/opt/gen/sigma/ide/sigma-ide --lsp --test --cwd /home/pepeiborra/si_sigma +RTS -A32M -I0 -s/tmp/sigma-ide-benchmark-large.gcStats 
 390,443,605,936 bytes allocated in the heap
  74,427,647,744 bytes copied during GC
  17,896,901,816 bytes maximum residency (14 sample(s))
     560,837,448 bytes maximum slop
           17067 MB total memory in use (0 MB lost due to fragmentation)

                                     Tot time (elapsed)  Avg pause  Max pause
  Gen  0       929 colls,   928 par   784.410s  19.680s     0.0212s    0.5344s
  Gen  1        14 colls,    12 par   478.783s  34.307s     2.4505s    21.2495s

  Parallel GC work balance: 86.81% (serial 0%, perfect 100%)

  TASKS: 284 (7 bound, 264 peak workers (277 total), using -N40)

  SPARKS: 0 (0 converted, 0 overflowed, 0 dud, 0 GC'd, 0 fizzled)

  INIT    time    0.001s  (  0.099s elapsed)
  MUT     time  718.973s  (177.425s elapsed)
  GC      time  1263.193s  ( 53.987s elapsed)
  EXIT    time    0.084s  (  0.009s elapsed)
  Total   time  1982.251s  (231.521s elapsed)

  Alloc rate    543,057,234 bytes per MUT second

  Productivity  36.3% of total user, 76.6% of total elapsed

[wz1000]

I had another look at this and fixed a couple of leaks and it seems like this patch is a big win for performance in most common cases.

There are two options in RulesConfig which can have an impact on performance for large projects:

1. fullModuleGraph(called checkForImportCycles in baseline)
2. enableTemplateHaskell

Both of these options are enabled by default.

In baseline HLS, checkForImportCycles and enableTemplateHaskell were largely independent, checkForImportCycles controlled if on each call to ghcSessionDepsDefinition, we recomputed the entire module graph below the file (a quadratic if not exponential process because of all the duplicated work).

With the patch, checkForImportCycles results in calls to GetModuleGraph instead, avoiding duplicated work.

enableTemplateHaskell always entails calls to GetModuleGraph because of the way we need to decide if anything above a file in the graph for the whole project requires this file to be compiled.

With the patch, either of these options when enabled result in calls to GetModuleGraph.

I will be testing two configurations, NOGRAPH={fullModuleGraph=False, enableTemplateHaskell=False} and GRAPH={fullModuleGraph=True, enableTemplateHaskell=True}, as these are the two canonical configurations that decide if GetModuleGraph is called.

I tested this patch with a project generated via the following script:

#!/usr/bin/env bash
# Generate $DEPTH layers of modules with $WIDTH modules on each layer
# Every module on layer N imports all the modules on layer N-1
# MultiLayerModules.hs imports all the modules from the last layer
DEPTH=15
WIDTH=40
for i in $(seq -w 1 $WIDTH); do
  echo "module DummyLevel0M$i where" > DummyLevel0M$i.hs;
done
for l in $(seq 1 $DEPTH); do
  for i in $(seq -w 1 $WIDTH); do
    echo "module DummyLevel${l}M$i where" > DummyLevel${l}M$i.hs;
    for j in $(seq -w 1 $WIDTH); do
      echo "import DummyLevel$((l-1))M$j" >> DummyLevel${l}M$i.hs;
    done
  done
done
echo "module MultiLayerModules where" > MultiLayerModules.hs
for j in $(seq -w 1 $WIDTH); do
  echo "import DummyLevel${DEPTH}M$j" >> MultiLayerModules.hs;
done

I focused testing on the two extremes, DummyLevel0M1 (which imports nothing) and MultiLayerModules which imports everything.

Here are the results of running haskell-language-server from the command line on these files:

commit	config	file	time	bytes allocated	peak memory
master	GRAPH	MultiLayerModules	17.288	47,771,227,040 bytes	1035 MiB
wip/module-graph-mem	GRAPH	MultiLayerModules	4.222s	14,076,683,792 bytes	712 MiB
master	NOGRAPH	MultiLayerModules	6.318s	15,325,017,376 bytes	952 MiB
wip/module-graph-mem	NOGRAPH	MultiLayerModules	6.126s	15,328,584,272 bytes	960 MiB
master	GRAPH	DummyLevel0M1	0.396s	572,811,704 bytes	161 MiB
wip/module-graph-mem	GRAPH	DummyLevel0M1	1.205s	4,113,919,264 bytes	302 MiB
master	NOGRAPH	DummyLevel0M1	0.397s	572,832,632 bytes	161 MiB
wip/module-graph-mem	NOGRAPH	DummyLevel0M1	0.397s	572,799,488 bytes	161 MiB

So this branch is much faster in the (default) GRAPH config for MultiLayerModules, slower for DummyLevel0M1, and equivalent in the NOGRAPH config for both files.

However, this is not the full story for the GRAPH config on DummyLevel0M1. Calling haskell-language-server on a file from the command line only runs the TypeCheck, GetHieAst and GenerateCore rules. None of these rules require us to know if we need to compile a file. It is only GetModIface that forces us to compute the full module graph in HLS master.
A more realistic test will then force at least one GetModIface call.

Testing DummyLevel1M1, one level up, so that GetModuleGraph is also called due to needing GetModIface on DummyLevel0M*, we get the following results:

commit	config	file	time	bytes allocated	peak memory
master	GRAPH	DummyLevel1M1	1.456s	4,531,463,816 bytes	416 MiB
wip/module-graph-mem	GRAPH	DummyLevel1M1	1.461s	4,502,678,944 bytes	383 MiB

Changing the haskell-language-server commandline check command to instead call GetModIface on its arguments also equalises the performance of both branches on DummyLevel0M1

And we see that this branch is equivalent to or outperforms master for most realistic scenarios and configurations.

[pepeiborra]

Thanks for pushing ahead with this branch and sharing the revised benchmarks!

My only concern is that the benchmarks specifically test startup scenarios, and I would like to see bench numbers for the edit-module-headers scenario that is expected to regress:

this could be solved easily be re-introducing
a version of GetDependencyInformation with early cutoff that essentially returns the result of GetModuleGraph
but includes the hash of only the ModSummarys in the downward dependency closure of the file.

Did you look into this?

[wz1000]

@pepeiborra I've added a benchmark to edit the header and also added my script to the benchmarks.

After benchmarking, I made a few more rules early cutoff, this dramatically improves edit performance for all scenarios other than the DummyLevel0M01 case.

I tested three commits, baseline(master), module-graph(this patch without improvements to early cutoff) and module-graph-cutoff(this patch). Here are the results of benchmarking:

DummyLevel0M1:

version               configuration   name          success   samples   startup   setup   userT   delayedT   1stBuildT   avgPerRespT   totalT   rulesBuilt   rulesChanged   rulesVisited   rulesTotal   ruleEdges   ghcRebuilds   maxResidency   allocatedBytes
baseline              All             edit-header   True      50        0.32      0.00    0.50    66.47      66.46       0.01          66.97    10           9              13             11504        572850      43            912MB          51438MB
module-graph          All             edit-header   True      50        0.13      0.00    0.45    3.05       3.05        0.01          3.51     10           9              14             10906        164129      1             389MB          17112MB
module-graph-cutoff   All             edit-header   True      50        0.12      0.00    0.44    3.23       3.22        0.01          3.67     10           9              15             11467        141453      82            423MB          17867MB
baseline              All             edit          True      50        0.25      0.00    0.49    67.01      67.00       0.01          67.51    10           9              13             11546        572808      1             849MB          50631MB
module-graph          All             edit          True      50        0.12      0.00    0.46    3.15       3.14        0.01          3.61     10           9              14             10826        164209      81            399MB          17982MB
module-graph-cutoff   All             edit          True      50        0.09      0.00    0.40    3.02       3.01        0.01          3.42     10           9              15             11467        141453      82            423MB          17754MB

DummyLevel0M01:

version               configuration   name          success   samples   startup   setup   userT   delayedT   1stBuildT   avgPerRespT   totalT   rulesBuilt   rulesChanged   rulesVisited   rulesTotal   ruleEdges   ghcRebuilds   maxResidency   allocatedBytes
baseline              All             edit-header   True      50        0.19      0.00    34.70   66.20      66.19       0.71          100.91   651          9              775            11467        572886      81            781MB          71020MB
module-graph          All             edit-header   True      50        0.11      0.00    34.50   2.95       2.98        0.70          37.45    651          9              855            10906        164129      1             389MB          37691MB
module-graph-cutoff   All             edit-header   True      50        0.14      0.00    2.26    3.06       3.05        0.05          5.33     10           9              856            11469        141450      81            428MB          27791MB
baseline              All             edit          True      50        0.20      0.00    33.97   65.32      65.31       0.69          99.30    651          9              775            11546        572808      1             852MB          70324MB
module-graph          All             edit          True      50        0.12      0.00    36.88   3.26       3.25        0.75          40.14    651          9              855            10826        164208      82            389MB          38301MB
module-graph-cutoff   All             edit          True      50        0.28      0.00    2.22    2.95       2.94        0.05          5.18     10           9              856            11548        141372      1             429MB          27183MB

MultiLevelModules:

version               configuration   name          success   samples   startup   setup   userT   delayedT   1stBuildT   avgPerRespT   totalT   rulesBuilt   rulesChanged   rulesVisited   rulesTotal   ruleEdges   ghcRebuilds   maxResidency   allocatedBytes
baseline              All             edit-header   True      50        0.25      0.00    55.89    69.19      69.18       0.38          125.08   668          22             4393           11550        572758      180           865MB          205450MB
module-graph          All             edit-header   True      50        0.16      0.00    59.90    2.83       2.82        0.41          62.73    665          20             3867           10910        164158      104           628MB          103989MB
module-graph-cutoff   All             edit-header   True      50        0.21      0.00    16.37    3.16       3.15        0.11          19.53    19           16             3861           11552        141322      182           578MB          98625MB
baseline              All             edit          True      50        0.26      0.00    118.19   66.16      66.15       0.80          184.35   1871         628            4394           11550        572238      101           1221MB         685464MB
module-graph          All             edit          True      50        0.18      0.00    99.24    3.21       3.21        0.68          102.46   1871         628            3871           10910        163559      180           1081MB         517165MB
module-graph-cutoff   All             edit          True      50        0.27      0.00    64.15    2.98       3.03        0.44          67.14    1221         621            3861           11554        140802      134           1097MB         530448MB

[pepeiborra]

Why is the fingerprint based on the module summaries? I can picture changed to module summaries that would not alter the module graph

[wz1000]

yes, this may be too course. I can try to implement a finer grained hash, though I'm not sure how much it would improve things.

[wz1000]

I don't think we want to do this because now the dependency information contains a GHC.ModuleGraph which is essentially a [ModSummary].

[pepeiborra]

We may still want a per module early cutoff right? The new benchmark scenario Edit-header doesn't alter the module graph, so it doesn't tell us what happens when a portion of the module graph is altered. Previously it would only dirty the modules in that portion, whereas now (even with the ModuleGraph early cutoff) it will dirty all the modules.

Concretely, I'm suggesting to keep GetDependencyInformation rule, reimplemented as a lookup on GetModuleGraph with early cutoff. I thought this was your original proposal. I might be wrong and this might be unnecessary, only benchmarks can tell

[wz1000]

We may still want a per module early cutoff right? The new benchmark scenario Edit-header doesn't alter the module graph, so it doesn't tell us what happens when a portion of the module graph is altered. Previously it would only dirty the modules in that portion, whereas now (even with the ModuleGraph early cutoff) it will dirty all the modules.

Concretely, I'm suggesting to keep GetDependencyInformation rule, reimplemented as a lookup on GetModuleGraph with early cutoff. I thought this was your original proposal. I might be wrong and this might be unnecessary, only benchmarks can tell

GetDependencyInformation was only used by ReportImportCycles - now I've made ReportImportCycles early cutoff (with a perfect hash, since the result is just unit). So ReportImportCycles can never dirty any modules. However, GetModuleGraph can still dirty ReportImportCycles. I'm not sure if adding a indirection to GetDependencyInformation in between would really help things, since inspecting if the module graph contains a cycle including the given file should not be too slow, certainly not slower than recomputing the hash for a restricted subset of the module graph (as we would need to do to implement GetDependencyInformation as you suggest).

Do you still think we need GetDependencyInformation?

[wz1000]

@pepeiborra do you think we need this setSomethingModified call after calling unqueueForEvaluation?