Ghcide: Use interface files (*.hi) when available

[pepeiborra]

Interface files could help speed up loading times and reduce memory usage

[ocharles]

Wouldn't this risk ghcide using out-of-date information, or am I misunderstanding the suggestion? The only .hi files we have are from cabal build, which is possibly much more out of date than what ghcide can see. Or is there a way to know when a .hi is stale (that's faster than just building a new one)?

[pepeiborra]

Interface files would only be used for dependencies, not for files being edited, and to determine staleness I was thinking just comparing timestamps with the matching source file.

[pepeiborra]

Teamed up with @mpickering and @lazamar working on this during the Bristol Hackathon. Current state: ghcide will use interface files in the typechecking rule when available. On the GHC codebase, loading the GHC module uses "only" 3GB as opposed to 5.6GB as it used to, and starts up significantly more quickly.
Because interface files carry less information, goto definition is not available for names in modules loaded from interface files. This can be remediated by falling back to .hie files, but these only exist in GHC >= 8.8.

Branch is https://github.com/pepeiborra/ghcide/tree/interface-files-rebased

Things missing:

• [x] 1 test failure involving Template Haskell
• [x] Interface file loading - stale checks
• [x] Interface file loading - way checks
• [x] Interface file type checking in the GHC codebase
• [x] Automatic .hi file generation after typechecking
• [x] Goto definition using .hie files needs extending to work for home modules
• [x] Stale check for .hie files
• [x] Enable feature only if GHC >= 8.8
• [x] Automatic .hie file generation after typechecking
• [x] Force generation of missing .hie files
• [x] Haddocks for home modules compiled with -haddock are not available with interface files
• [x] Fix .hi and .hie file generation to reuse the HscEnv from typechecking
• [x] Tune down the logging output and/or replace with error handling
• [x] Use diagnostics to flag when interface files are not being used
• [x] Write tests for goto definition and/or hover involving reexported symbols,
• [ ] Optimization: avoid using the ParsedModule for modules loaded from interface files for even faster startup and bigger memory savings. WIP
• [x] Optimization: replace dependency on TypeCheck rule in ByteCode rule with GetModIface
• [x] Optional: generate custom .hie-like files in GHC versions <8.8.x to enable the use of interface files

[pepeiborra]

New branch: https://github.com/pepeiborra/ghcide/tree/interface-files-rebased-again

[mpickering]

I tried the latest branch and receive the following error when trying to load GHC.

Step 6/6: Type checking the files
typecheckIface
Declaration for ComponentId
Constructor ComponentId
Constructor ComponentId arg_tys:
  attempting to use module ‘ghc-8.8.1:FastString’ (compiler/utils/FastString.hs) which is not loaded
File:     compiler/basicTypes/Module.hs
Hidden:   no
Range:    1:0-100001:0
Source:   typecheck
Severity: DsError
Message:  Program error: Cannot continue after interface file error

[mpickering]

The check to use interface files also needs to check that the interface files are built for the right way I think. I pointed the hidir to a random build folder I had and was greeted by many errors. Ideally for me ghcide will write the interface files itself.

File:     .hie-bios/stage0/compiler/build/Config.hs
Hidden:   no
Range:    1:0-100001:0
Source:   compiler
Severity: DsError
Message:  mismatched interface file ways (wanted "", got "dyn")

[pepeiborra]

I tried the latest branch and receive the following error when trying to load GHC.

Step 6/6: Type checking the files
typecheckIface
Declaration for ComponentId
Constructor ComponentId
Constructor ComponentId arg_tys:
  attempting to use module ‘ghc-8.8.1:FastString’ (compiler/utils/FastString.hs) which is not loaded
File:     compiler/basicTypes/Module.hs
Hidden:   no
Range:    1:0-100001:0
Source:   typecheck
Severity: DsError
Message:  Program error: Cannot continue after interface file error

I'm not terribly surprised. Even though it works fine for the ghcide codebase and at work, the code for setting up the typechecking environment with ModIface values is very crude and getting it to work with the GHC codebase will be a challenge. Help welcome!

[pepeiborra]

The check to use interface files also needs to check that the interface files are built for the right way I think. I pointed the hidir to a random build folder I had and was greeted by many errors. Ideally for me ghcide will write the interface files itself.

File:     .hie-bios/stage0/compiler/build/Config.hs
Hidden:   no
Range:    1:0-100001:0
Source:   compiler
Severity: DsError
Message:  mismatched interface file ways (wanted "", got "dyn")

Yes, the logic for loading the interface files one of the items that needs some work. Not only to check they are built the right way but also that they are not stale.

About generating them ourselves, how would that work? Ideally we would like to reuse the ones generated by the build system and generate fresh ones when needed.

EDIT: maybe generate .hi files as a by-product of type-checking.

[mpickering]

I can try to help with this branch but perhaps not until the weekend. Let me know if there are some aspects you plan to work on still or which bits are safe to touch.

[pepeiborra]

Thanks Matthew, that would be great. What I plan to do next:

1. Extend the README with a couple of notes around .hie and .hi files and the expected Cabal setup
2. Help needed: improve the loading of interface files and the typechecking complex projects like GHC.
3. Break the changes down in smaller, self-contained, reviewable branches. Some of these could be:
4. The improvements to .hie files and getDefinition
5. The rules for loading .hi files and GetModIface
6. Typechecking using GetModIface
7. Testing. The current test suite passes which means it probably needs extending. For instance, getDefinition is not tested for reexported names.

[mpickering]

I am going to try to analyse the issue which is affecting GHC tonight.

[pepeiborra]

You probably want to start with the Development.IDE.Core.Compile.loadInterface function, we are not doing any recompilation checking. Next bit would be the typecheckModule function in the same module.

[mpickering]

The setupEnv' function looks pretty dodgy but I don't know exactly what it is trying to do. Sorting the modules so that hs-boot modules are first doesn't seem right under any interpretation?

The latest error I am getting after some fiddling is

Variable not in scope: pprIfaceType :: IfaceType -> SDoc

So for some reason we are using the interface file from the boot file rather than the actual module when trying to compile a module. It's not clear to me how this could occur yet, I want to verify the GetModIface rule works as expected.

[mpickering]

I also don't think the getHiFileRule works for hs-boot interface files as the ml_hi_file location is set incorrect to the path to the interface file for the source module.

[pepeiborra]

My understanding is that the setupEnv' function is trying to set up an environment E for type checking a module A so that all the dependencies of A are loaded in E. This code was already working in ghcide, so I would hope it does the right thing for typechecking with interface files too.

Did you try extending getHiFileRule to fix the ml_hi_file path for hs-boot modules?

[mpickering]

After far too much time I realised what the issue was which was causing the load failure in GHC. We needed to add a proper edge between a .hs and .hs-boot file when calculating the dependency order so that the .hs file would appear later in the load order.

This also explains why the code in setupEnv was trying to sort the dependencies in order to make the hs-boot files get loaded first. This is incorrect in general so I removed that code and renamed the function.

I am not sure of the reason that the ".hi" field for hs-boot files is set to the .hi file for the main module. I suppose it might be because you can use the interface file for an already compiled module rather than a hs-boot file in some situations? I made the change to look for hi-boot anyway.

Changes are on the branch - https://github.com/pepeiborra/ghcide/tree/interface-files-rebased-again

[mpickering]

On the latest branch I get errors like this.

File:     compiler/nativeGen/RegAlloc/Graph/TrivColorable.hs
Hidden:   no
Range:    1:0-100001:0
Source:   compiler
Severity: DsError
Message: 
  /home/matt/ghc/test-hi/RegAlloc/Graph: openTempFile: does not exist (No such file or directory)
Files that failed:
 * ghc/Main.hs

[pepeiborra]

That error could be coming from the code paths that generate .hi and .hie files after typechecking. These are called from typecheckRule. I’ll take a look tomorrow if you don’t get there before me

[pepeiborra]

@mpickering I've pushed some changes to reuse GHC smarts for interface recompilation checking.
There is a problem with the include paths computed by hie-bios - they are absolute, whereas Cabal ones are relative. This defeats the fingerprintDynFlags in iface/FlagChecker.hs.
What to do?

Example:

[nix-shell:~/scratch/ghcide]$ cabal build -v3
(...)
/nix/store/rd2hayihmvzryk3i9mb216kdz1w2w08h-ghc-8.8.1-with-packages/bin/ghc --make ... -isrc ...

[nix-shell:~/scratch/ghcide]$ hie-bios flags src/Development/IDE/Spans/Common.hs 
Options: [(..),"-i/home/pepe/scratch/ghcide/src",...

[mpickering]

@pepeiborra Are you talking about reusing interface files or what exactly is the problem?

[pepeiborra]

Reusing interface files created by Cabal and using the GHC recompilation checker to verify whether an interface file is up to date

[mpickering]

The problem with the cabal include paths is that they are relative to the directory where the cabal file is located, not the root of the project, where the cabal.project is located. ghcide runs in the project root direct and hence needs to modify the include paths.

I still think the best thing is to get ghcide to generate them and reuse them between runs, this will just be the first of many issues where the interfaces don't match up properly and cause subtle issues.

[pepeiborra]

Ghcide is already generating them. At the moment however they end up in the Cabal dist folder, triggering recompilation on cabal build since the flags dont match for the reasons just mentioned. So the current situation is a no-go: either handle our own interface files or fix the flags to match cabal.

What other issues can you think of? GHC recompilation logic is extensive and battle tested, and I just pushed a change to use it.

What I would suggest:

1. Fix the code in GHC to use absolute paths for flag fingerprints
2. Use a local cache of interface files until the GHC fix above is released

[mpickering]

If ghcide sets flags like -O0 or -fno-code then the interface files are going to be incompatible as the unfolding information will be different, for example. If we start writing into the cabal dist directory then if you are also using cabal on the command line it will cause additional compilation.

[pepeiborra]

That’s a good point - even if we fix flags fingerprints, the optimization settings used by ghcide will probably be different.

[mpickering]

A related question, we should also manage the hiedir, when I last tried the branch is resulted in a lot of .hie files littering the build tree.

[pepeiborra]

Sounds good to me. What’s the best way to do that, in ghcide or hie-bios?

[mpickering]

The hidir used to be set in hie-bios to something in XDG_CACHE_DIR but we changed this in Bristol. I think it's better to deal with this in ghcide now we also need to set the hiedir?

[pepeiborra]

I’m going to be making these changes today, and will take a look at the ghc issues too if I have time.

[pepeiborra]

Just pushed a couple of changes to the branch and now main/GHC.hs loads successfully in VSCode using only 2.2GB. Go to definition and hover work. The main problem was that we were not writing hi-boot files to the right file path.

A couple of things:

• ghcide now manages its own .hi files - which means it doesn't reuse anything from Hadrian. In my laptop, ghcide runs out of memory a couple of times before stabilizing once all the interface files have been produced
• The cache for ghcide lives under XDG_CACHE_DIR, similar to what hie-bios does. Because VSCode seems to customize XDG_CACHE_DIR, ghcide-on-vscode and ghcide-cli do not share the same cache folder
• ghcide-cli fails to load main/GHC.hs, producing various kinds of errors. It looks like .hi files for modules with a .hs-boot file never get generated for some reason. I'm not sure what to do about this.

[mpickering]

How are you invoking ghcide-cli mode?

How much memory does your laptop have?

It seems very bizarre that vscode modifies XDG_CACHE_DIR, are you sure about that? What does it set it to?

[pepeiborra]

/home/pepe/scratch/ghcide/dist-newstyle/build/x86_64-linux/ghc-8.8.1/ghcide-0.0.6/x/ghcide/build/ghcide/ghcide compiler/main/GHC.hs

Laptop has 8gb available for ghcide

/home/pepe/snap/code/common/.cache/ so probably Ubuntu is modifying it

[mpickering]

After a brief search I can't find any issues complaining about this, I'll also try tomorrow.

[mpickering]

I was thinking yesterday that the way the rules are structured there will be a lot of repeated work. For each module we will need to typecheck it, this means that loadDepModule will be called on all transitive dependencies of every module which will cause a quadratic number of module loads. Did the old architecture avoid this issue?

In GhcMake each module is only loaded once because the interface files are created as a byproduct of loading the transitive dependency of the target module.

[pepeiborra]

I think this is part of how ghcide is designed to work. GhcMake is too monolithic and not a good fit, Shake allows for more control. The quadratic number of loads is present in the old architecture too, I guess it's not a problem because loadModuleHome is cheap. loadDepModule does a bit more work - it needs to synthetize a ModDetails from ModIface- but I don't know whether that's expensive or not. If necessary, it could probably be avoided by storing the ModDetails in the HiFileResult.

[mpickering]

I found one place where the .hi rather than .hi-boot file was being looked for again (and pushed a fix). We should come up with a more permanent way of fixing this issue though, by making sure the .hi field of ModLocation points to the .hi-boot file?

I was unsure about doing this because though because I assumed there must be a reason that it was always set to the .hi file for a module.

[mpickering]

I can load GHC now on an empty or full cache.

Empty cache: 10gb - 1m 4s real time
Populated cache: 2.5gb - 26.9s real time

[pepeiborra]

That's consistent with what I see. Is that using ghcide cli?

Can you try to push your change again? It's not there

About hi-boot files, this is a GHC problem, isn't it? We could work around it by mangling the ModLocations created by parsing

[mpickering]

I pushed it - 074b3495c76117f15a0b0f37a8bd2abc4a36945f

[mpickering]

Yes, I was using the CLI.

It's not a "problem" in GHC, as there is a comment saying that this is how it's supposed to work.

https://github.com/ghc/ghc/blob/master/compiler/basicTypes/Module.hs#L266

My question is, why is this how it's supposed to work..

[mpickering]

After reading the source code for about a minute I would rather not know exactly how this works, at least for now.

[pepeiborra]

Intense clean up in a new branch: https://github.com/pepeiborra/ghcide/tree/interface-files-rebased-3

This looks almost ready for merging upstream.

[mpickering]

Thanks @pepeiborra I will test again your rebase tomorrow with a proper vscode setup.

Is there anything else we need to look into before making a PR? You've done great work on this patch.

[pepeiborra]

Thanks, it's been a team effort!

The minimum bits left to do before merging, I think, are:

• Look for regressions esp. on GHC 8.6 where interface files are not enabled,
• Possibly add a cl flag or a plugin option to disable interface files in 8.8
• Write tests to verify that interface files are being used
• Write tests for goto definition and/or hover involving reexported symbols,
• Tune down the logging output
• More tests

[mpickering]

Is there some way to evict the TypecheckedModules from memory as soon as they are used to produce the interface files? The first load just uses too much memory and my computer starts swapping.

There is something wrong with goto definition on GHC's code base, did you try that? It seems to compute the wrong path so peeking and jumping don't work.

[mpickering]

Some jumping does work, ie from ToolSettings in DynFlags to the ToolSettings module.

Jumping from Main.hs to cBooterVersion doesn't work though. It seems to be passed a relative rather than an absolute path.

[pepeiborra]

Evicting from the Shake graph sounds complicated, but calling typecheckModule directly from getModIfaceRule instead of going through use_ Typecheck would do the trick.

No idea what's the problem with jumping. Does it normally work with ghcide 0.1.0 ?

[ndmitchell]

The Shake graph assumes anyone can grab a node at any point because its monadic and open world. Avoiding the intermediate node is best.

[mpickering]

Some more peformance testing, loading ghc/Main.hs.

Prepatch: 33.62 load - 1.23s get definition
After (no cache): 55.10s load - 3.35s get definition
After (hot cache): 24.52 load - 2.4s get definition

I imagine get definition performance is now worse because shake scales linearly with the number of dependencies a rule has.

[pepeiborra]

get definition now uses .hie files, which need to be located, loaded and queried, whereas before it would use the SrcLoc embedded in the Id itself.

[mpickering]

I will do a shake profile of this but it should not take 1.2 to read and deserialise a single file. If it does then we can speed up the query by loading hie files into something like HieDB.

[mpickering]

Profile: https://mpickering.github.io//ide-20200204-220537-00002-2.20.html

Looks like this problem only really shows up with GHC because of the large number of include paths. 16000 rules fire just for a single go to definition request. How is the performance on your code base?

[pepeiborra]

I have noticed some slow go to definition performance in our codebase too, but it only happens in modules with tons of transitive dependencies. Definitely something to investigate further, but possibly not in this patch.

Your profile does not look right. GetFileExists depends on alwaysRerun, but I removed that dependency in HEAD. It's only present if your lsp-client cannot provide watched files, check the code i the FileExists module.

[mpickering]

My client is the CLI. It's unfortunate if it can't be used accurately for profiling.

[pepeiborra]

You could special case the CLI in the FileExists module, but I think for Shake profiling, a real lsp client is more likely to produce realistic results.
I'm getting some puzzling interface loading errors at work:

GHC error in desugarer lookup in Ghci1:
  attempting to use module ‘main:Strats.Cloud.Query.Eval’ (src/Strats/Cloud/Query/Eval.hs) which is not loaded
GHC error in desugarer lookup in Ghci1:
  Can't find interface-file declaration for variable $tcResult
    Probable cause: bug in .hi-boot file, or inconsistent .hi file
    Use -ddump-if-trace to get an idea of which file caused the error

And

GHC error in desugarer lookup in Ghci1:
  attempting to use module ‘main:Strats.Cloud.Types.Misc’ (src/Strats/Cloud/Types/Misc.hs) which is not loaded

GHC error in desugarer lookup in Ghci1:
  Can't find interface-file declaration for variable $tcWriterT
    Probable cause: bug in .hi-boot file, or inconsistent .hi file
    Use -ddump-if-trace to get an idea of which file caused the error



md5-0eb466e6af3b548d38fefe5f587953dc



Completed (1275 files worked, 3 files failed)

[mpickering]

Are these errors coming from the initial loading or when trying to perform actions? I am assuming no Template Haskell so I'm wondering where the desugaring is being invoked from.

[mpickering]

My suspicion here is that you should use an HscEnv returned by typecheckModule when calling generateAndWriteHieFile rather than the one from GhcSession. The one passed into generateAndWriteHieFile will have no interface files loaded.

[mpickering]

The Shake graph assumes anyone can grab a node at any point because its monadic and open world. Avoiding the intermediate node is best.

Why does this mean that you can't force a node to be garbage collected? If someone else demands it later then the result could be recomputed?

Is the way you recommend to solve this to factor out the code from typeCheckRule into it's own function which can be called directly from the ModIface rule?

[mpickering]

New branch with the change that GetModIface doesn't call TypeCheck.

https://github.com/pepeiborra/ghcide/tree/interface-files-0.1.0.0

I noticed that the ByteCode rule will call TypeCheck on all transitive dependencies as well. We never noticed this so far because neither of our projects use Template Haskell. Could be something to be improved in future.

[pepeiborra]

On the errors, we do have a bit of TH in that area, introduced recently. I'll check if removing it makes the error go away, just to confirm.

I'll give your branch a go in our work codebase, thanks!

[pepeiborra]

I found a bug in getSpanInfo, it was not loading the dependencies in the right order. I'll prepare a fix later today.

I also noticed that disabling the use of interface files, by not providing the --use-interface-files flag, doesn't help with the errors as the code for typechecking is new. This probably means that the flag is pointless and redundant, as there is no simple way to turn our changes off.

For the TH errors, a warm interface files cache made them go away, which hopefully means it's a non issue. Unpatched ghcide 0.1.0 did show them too so not a regression hopefully

Benchmarks loading a module with many deps on the cli:

• ghcide 0.1.0: 60s 3.4GB max residency
• interface-files-0.1.0: 69s 1.4GB max residency
• interface-files-0.1.0 --use-interface-files cold: 68s 1.5GB max residency
• interface-files-0.1.0 --use-interface-files warm: 27s 0.39GB max residency

[pepeiborra]

My suspicion here is that you should use an HscEnv returned by typecheckModule when calling generateAndWriteHieFile rather than the one from GhcSession. The one passed into generateAndWriteHieFile will have no interface files loaded.

should fix this before wrapping up

[alanz]

Does this effort impact on multi-component support in any way? As in make it easier once it lands?

[cocreature]

Does this effort impact on multi-component support in any way? As in make it easier once it lands?

Not really, this is orthogonal.

[alanz]

Ok, I thought so, but just saw a comment about using specific HscEnv versions, which I understand is one of the issues for multi component.

[pepeiborra]

Pushed a couple more changes to the branch, including the bugfix to getSpansInfo. Plan is to wrap up with tests and real world usage, with a PR upstream by this weekend.

@cocreature do you prefer many small sized PRs or a large one?

[pepeiborra]

I have been using this patch in our codebase without issues in GHC 8.8.2 today
@ocharles have you had a chance to test with your stuff?
@ndmitchell does it handle yours?

[ocharles]

I haven't had a chance to test yet I'm afraid, and won't be able to until at least Tuesday. I'd say don't hold off on my behalf

[mpickering]

@pepeiborra It occurred to me that we could still support ghc-8.6.5 with this patch if we implemented the "hie" files ourself for the small part that we use. To be precise we can implement the stub in the Compat module with our own serialisation/deserialisation logic. The hie_exports field is created just by calling the mkIfaceExports function so it should be quite straightforward.

There are still many people using ghc-8.6.5 so it seems worth the effort to me.

[ndmitchell]

@pepeiborra actually trying this out on the code base I am interested in will be a multimonth project in itself - there are lots of very custom things in our environment. I'll report back, but perhaps around the middle of the year :)

[mpickering]

@pepeiborra How does jump to definition work with the HIE files? It looks like it looks in the exports filed, which stores AvailInfo, but then calls nameSrcSpan, which surely is not an helpful span if the HIE file is read from disk as it will reuse the normal serialisation logic for Name.

(It works for me when the cache is cold)

[mpickering]

Pepe - see this branch for a version of "hie" files for 8.6.5. https://github.com/mpickering/ghcide/tree/wip/interface-files-hie-8.6.5

It works in my testing for cache loads as well.

[pepeiborra]

@pepeiborra How does jump to definition work with the HIE files? It looks like it looks in the exports filed, which stores AvailInfo, but then calls nameSrcSpan, which surely is not an helpful span if the HIE file is read from disk as it will reuse the normal serialisation logic for Name.

(It works for me when the cache is cold)

The HIE file is indeed read from disk, it works because .hie files use a custom serialiser for Name, which must preserve span data. See putName in the HieBin module.

Can you use the same trick in your branch ?

[pepeiborra]

New rebased branch: https://github.com/pepeiborra/ghcide/tree/interface-files-0.1.0.0-rebased

This includes two bugfixes to the recent change to call typecheckModule directly from the GetModIface rule:

• depend on TypeCheck nodes for files of interest, to avoid having two sources of ModIface in the graph
• propagate the diagnostics produced by typecheckModule

As well as test fixes to cope with the new diagnostics produced by loading interface files.

The new branch does not yet include the custom .hie files in 8.6.5 as the branch does not typecheck for me in 8.8.1 - @mpickering do you want to apply the custom Name serialiser trick so that both versions can be driven in the the same way?

I've also started extracting PRs upstream.

[mpickering]

@pepeiborra Using the functions getWithUserData means that I didn't have to copy large amounts of code from GHC into ghcide. I will modify the patch so that the 8.8 version provides a function which converts a [AvailInfo] into [(SrcSpan, Name)] so that it is compatible with the simpler 8.6 version.

[mpickering]

The branch now uses interface files for all GHC versions. What have you got left to do Pepe?

[pepeiborra]

I'm busy getting the patches accepted. So far there's been 3 PRs merged, one more is underway, and only a few more after that.

In terms of future work, there's bypassing parsing altogether that @lazamar has been working on: https://github.com/pepeiborra/ghcide/pull/1