Completions need not depend on typecheck of the current file

[pepeiborra]

A completion request immediately after an edit is slow because currently ProduceCompletions depends on Typecheck, for very little reason.

Following the plan in #655, we split ProduceCompletions in two rules, for local and non local (imported) completions.

• The non local completions reuse all the existing code paths, but depend only on ModSummary, which has early cutoff and thus do not recompute during module body edits.
• The local completions are extracted from the parsed module, reusing type signatures where available for type information.

The speed up is visible in the completions after edit benchmark:

The rest of benchmarks stay the same:

pepe:~/scratch/ghcide$ column -ts,  bench-hist/results.csv 
version    name                         success   samples   startup              setup                experiment            maxResidency
upstream   hover                        True      100       10.137035238000001   0.0                  0.43737398800000005   170MB
upstream   edit                         True      100       11.158884498         0.0                  28.272691534000003    168MB
upstream   getDefinition                True      100       10.016039599         0.0                  0.8219402480000001    172MB
upstream   hover after edit             True      100       10.079089589         0.0                  35.162631657000006    170MB
upstream   completions after edit       True      100       10.024101074         0.0                  38.018306304          173MB
upstream   code actions                 True      100       10.09370142          0.59431742           0.7939273480000001    197MB
upstream   code actions after edit      True      100       11.408190008         0.0                  28.982654969000002    197MB
upstream   documentSymbols after edit   True      100       11.608852644         0.0                  4.428313810000001     169MB
HEAD       hover                        True      100       9.780013137000001    0.0                  0.43749480100000004   172MB
HEAD       edit                         True      100       10.349219719         0.0                  27.572064352          170MB
HEAD       getDefinition                True      100       9.836214871000001    0.0                  0.79330206            174MB
HEAD       hover after edit             True      100       10.076800354000001   0.0                  34.656411464          172MB
HEAD       completions after edit       True      100       9.792341248000001    0.0                  28.580005848000003    178MB
HEAD       code actions                 True      100       9.904102277          0.5609964700000001   0.680103295           199MB
HEAD       code actions after edit      True      100       10.072807663         0.0                  28.421011763000003    200MB
HEAD       documentSymbols after edit   True      100       9.736263303000001    0.0                  4.49522224            171MB

Full benchmark data in https://github.com/pepeiborra/ghcide/tree/completions-benchmark/bench-hist

Closes #655

[cocreature]

Thanks for the PR!
I have to admit, I’m a bit skeptical if this is a good idea. It adds a fair amount of complexity, typechecking the module returned by parsing the header seems a bit fishy and wastes resources (parsing the header is cheap, I don’t know how expensive typechecking is) and looking at the graph it seems like a 20% speedup which is great but also doesn’t seem like it will make a huge difference.

[cocreature]

This needs some documentation (or maybe cachedDataProducer does). We are parsing only the header but then we also typecheck is which is at least surprising. Looking at the code the part that depends on the typechecked module is tcg_type_env and tcg_rdr_env. Afaik that will contain top-level definitions usually so we get less now and expect to get this from the local definitions?

[pepeiborra]

There is a comment about this in the ProduceCompletions rule a few lines above (line 60). Does it need expanding?

[pepeiborra]

Thanks for the PR!
I have to admit, I’m a bit skeptical if this is a good idea. It adds a fair amount of complexity, typechecking the module returned by parsing the header seems a bit fishy and wastes resources (parsing the header is cheap, I don’t know how expensive typechecking is) and looking at the graph it seems like a 20% speedup which is great but also doesn’t seem like it will make a huge difference.

To address your concerns about typechecking the module header:

• It allows to make this a minimal change, as opposed to rewriting the completions logic.
• Typechecking an empty module is very very cheap,
• It only happens when the module header changes, which is not the case for most completions (unless the user is asking for an import completion).

I suspect the 20% improvement is more significant than it looks like. If we break down the time spent in the completions handler (including the shake session restart as this is completions after edit), I suspect it goes something like:

• 35% Shake overheads (including thread context switches)
• 15% ghcide overheads (progress reporting, diagnostics, etc.)
• 10% typechecking
• 10% lsp overheads (serialisation, normalization, etc)
• 30% actual completion logic

So a reduction of 20% in total time actually means that the completion logic is 66% more efficient, which will pay off once the other overheads go down.

[cocreature]

Alright, you managed to convince me that this is reasonable, thanks!