Fix performance of getFileExists #322
Conversation
We touch the file system only the first time. After that, we rely on the lsp client to tell us if a file is created or deleted Fixes #101
these are the same as the default extensions allowed in the library
|
Nice work. To attempt to summarise the code (so I can check I am reading it right), if the IDE supports dynamic registration of files to watch, then any time we look at a file, we register a listener for it. Seems sensible and smart, and if it fixes the problem, a nice and targeted fix. We should definitely fix lsp-test to follow the LSP spec here (maybe not immediately, but one day). It's exactly the kind of thing that is good to test! |
|
Thank you! I did a quick test run in vscode on the ghcide repository and this does greatly speed-up hover for me. On ghcide master hover takes between 0.50-0.90s to complete, and with this PR between 0.04-0.07s. In my case the reason for the slow hover on master are many file lookups in the |
The intended logic was to insert "exists" in the map followed by an event handler install. But somehow the insert got lost
|
Thank you @pepeiborra. Once this is released I can start to advertise using ghcide on GHC's code base more. |
|
@cocreature is there anything else needed here? |
|
Should be good to go I think, I’ll do a final review tomorrow when I’m back at work. |
|
Awesome, thank you so much for working on this! Sorry for the long review process. |
* Improve hover performance by speeding up getFileExists We touch the file system only the first time. After that, we rely on the lsp client to tell us if a file is created or deleted Fixes #101
* Improve hover performance by speeding up getFileExists We touch the file system only the first time. After that, we rely on the lsp client to tell us if a file is created or deleted Fixes haskell/ghcide#101
* Improve hover performance by speeding up getFileExists We touch the file system only the first time. After that, we rely on the lsp client to tell us if a file is created or deleted Fixes haskell/ghcide#101
* Improve hover performance by speeding up getFileExists We touch the file system only the first time. After that, we rely on the lsp client to tell us if a file is created or deleted Fixes haskell/ghcide#101
This is a minimal solution that does not entirely follow the plan in #101, focusing on GetFileExists instead of GetLocatedImports.
In my experiments it solves the problem, hopefully without breaking anything else. Checked with VSCode only.
Adding tests is tricky since lsp-test does not send DidChangeWatchedFiles notifications. I'm open to suggestions on what to do.
Leveraging DidChangeWatchedFiles notifications for other purposes, e.g. in the GetModificationTime rule, is left for future PRs.