Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

didChangedWatchedFiles notifications lock up emacs on startup #843

Closed
michaelpj opened this issue Sep 8, 2020 · 11 comments · Fixed by haskell/ghcide#831
Closed

didChangedWatchedFiles notifications lock up emacs on startup #843

michaelpj opened this issue Sep 8, 2020 · 11 comments · Fixed by haskell/ghcide#831

Comments

@michaelpj
Copy link
Collaborator

I observed this in HLS 0.3.0.0, but I'm pretty sure this is a ghcide issue, since this seems to be where we send the notifications.

When I start up a session in my (not ridiculously large) project, we send ~3k didChangedWatchedFiles notifications to the client. This locks up emacs completely until they are all processed, which it seems to do slowly, at a rate of ~10 responses a second (by tailing hie.log).

This is a pretty bad user experience. Clearly some of the fault is emacs' or lsp-mode's: 3k files to watch is really not that much, and I don't know how it's dealing with them so badly. Maybe we can do something upstream.

However, maybe we could do something to stress it out less. Some possibilities:

  • If the problem is large numbers of messages, we could debounce the notification queue and accumulate multiple watchers into a single didChangeWatchedFiles notification (it supports sending multiple watchers per notification). I suspect this would reduce the number of messages a lot.
  • If the problem is a large number of watchers, we might be able to use glob patterns to reduce the number of watchers a lot.
    • At least we seem to watch Foo.hs and Foo.lhs separately, which could be merged.
    • Doing <src>/**/*.hs for all the component's source folders might be an over-estimate, but it might be worth it if it's cheaper.
@pepeiborra
Copy link
Collaborator

Try with ghcide HEAD, it should set much fewer watches

@mrBliss
Copy link
Contributor

mrBliss commented Sep 9, 2020

@michaelpj I think that lsp-mode indeed exacerbates it: the lsp--file-process-event function, which process each event created by a watcher, does a linear scan over the watchers (see the -any?) of each project to find out which workspaces are affected by the event.

Since I only use a single workspace, I removed the check and let my single workspace process it unconditionally:

(defun lsp--file-process-event (session root-folder event)
  "Process file event."
  (let* ((changed-file (cl-third event))
         (event-numeric-kind (alist-get (cl-second event) lsp--file-change-type)))
    (->>
     session
     lsp-session-folder->servers
     (gethash root-folder)
     (seq-do (lambda (workspace)
               (with-lsp-workspace workspace
                   (lsp-notify
                    "workspace/didChangeWatchedFiles"
                    `((changes . [((type . ,event-numeric-kind)
                                   (uri . ,(lsp--path-to-uri changed-file)))])))))))))

Without this change, ghcide + lsp-mode is unusably slow for me.

@michaelpj
Copy link
Collaborator Author

@mrBliss have you opened an issue with lsp-mode about this? It sounds like you've got a reasonable case there!

@mrBliss
Copy link
Contributor

mrBliss commented Sep 9, 2020

@mrBliss have you opened an issue with lsp-mode about this? It sounds like you've got a reasonable case there!

The reason I haven't is because I thought that mainly ghcide was to blame. If other language servers also created a watcher per file, other people surely must have encountered this problem when working on non-trivial projects.

Also, I don't immediately see how lsp-mode can fix it, if it still wants to forward the event only to the affected workspace. I think that letting ghcide watch <src>/**/*.hs for all the component's source folders, as you suggest, is a much easier solution.

@michaelpj
Copy link
Collaborator Author

The maintainers might have an idea! We should at least give them the opportunity to close it wontfix ;) At least it would be helpful to get a clear signal of "no really, don't do this", if that's the case.

@michaelpj
Copy link
Collaborator Author

I made an issue anyway, let's see what they say.

@michaelpj
Copy link
Collaborator Author

It doesn't seem to be better on newer versions of ghcide. Still getting lots of multi-minute hangs. Looking at the source, it looks like we're going to send a lot of messages in fileExistsFast: https://github.com/haskell/ghcide/blob/51907fe47a74308ff5c04f9eccd124d292bbaa5f/src/Development/IDE/Core/FileExists.hs#L143

@michaelpj
Copy link
Collaborator Author

I've noticed some further odd behaviour. It seems like if we look at a file <src1>/A/B.hs, we "look" in all the other source folders too, and send watch notifications for those too. So a single file lookup sends N notifications! Times 2 for the .lhs as well.

This renders emacs almost unusable for me after I've opened a few components.

@pepeiborra
Copy link
Collaborator

That is how it works indeed. To resolve imported modules, ghcide has to consider all the possible locations where that module could exist, including all the import dirs and file extensions.

This is because Cabal/Stack cradles provide module targets that do not identify the path to the module. If you are lucky enough to be using a Bios cradle then do make sure to provide file targets; ghcide is smart enough to register only one watcher per file in most cases.

Changing the current behaviour to watch entire folders instead of specific files should be relatively easy to do, but it's not something I am planning to work on since my editor can handle the current behaviour with no problem at all.

@michaelpj
Copy link
Collaborator Author

Changing the current behaviour to watch entire folders instead of specific files should be relatively easy to do

Could you leave a few bullet points saying what you would likely try here? I might give it a go.

@pepeiborra
Copy link
Collaborator

You will need to modify the GetFileExists rule, which lives in https://github.com/haskell/ghcide/blob/master/src/Development/IDE/Core/FileExists.hs

The current implementation registers a watcher the first time it is called for a file F.

You want to replace that with a workspace-wide watcher for the file extensions in https://github.com/haskell/ghcide/blob/master/src/Development/IDE/Types/Options.hs#L61

The workspace-wide watcher should probably be registered by fileExistsRules

michaelpj referenced this issue in michaelpj/ghcide Sep 24, 2020
This prevents us from sending thousands of notifications to the client
on startup, which can lock up some clients like emacs. Instead we send
precisely one.

This has some consequences for the behaviour of the fast file existence
lookup, which I've noted in the code, alongside a description of how it
works (I spent a while figuring it out, I thought I might as well write
it down).

Fixes #776.
michaelpj referenced this issue in michaelpj/ghcide Sep 24, 2020
This prevents us from sending thousands of notifications to the client
on startup, which can lock up some clients like emacs. Instead we send
precisely one.

This has some consequences for the behaviour of the fast file existence
lookup, which I've noted in the code, alongside a description of how it
works (I spent a while figuring it out, I thought I might as well write
it down).

Fixes #776.
michaelpj referenced this issue in michaelpj/ghcide Sep 25, 2020
This prevents us from sending thousands of notifications to the client
on startup, which can lock up some clients like emacs. Instead we send
precisely one.

This has some consequences for the behaviour of the fast file existence
lookup, which I've noted in the code, alongside a description of how it
works (I spent a while figuring it out, I thought I might as well write
it down).

Fixes #776.
michaelpj referenced this issue in michaelpj/ghcide Sep 25, 2020
This prevents us from sending thousands of notifications to the client
on startup, which can lock up some clients like emacs. Instead we send
precisely one.

This has some consequences for the behaviour of the fast file existence
lookup, which I've noted in the code, alongside a description of how it
works (I spent a while figuring it out, I thought I might as well write
it down).

Fixes #776.
michaelpj referenced this issue in michaelpj/ghcide Sep 25, 2020
This prevents us from sending thousands of notifications to the client
on startup, which can lock up some clients like emacs. Instead we send
precisely one.

This has some consequences for the behaviour of the fast file existence
lookup, which I've noted in the code, alongside a description of how it
works (I spent a while figuring it out, I thought I might as well write
it down).

Fixes #776.
michaelpj referenced this issue in michaelpj/ghcide Sep 26, 2020
This prevents us from sending thousands of notifications to the client
on startup, which can lock up some clients like emacs. Instead we send
precisely one.

This has some consequences for the behaviour of the fast file existence
lookup, which I've noted in the code, alongside a description of how it
works (I spent a while figuring it out, I thought I might as well write
it down).

Fixes #776.
michaelpj referenced this issue in michaelpj/ghcide Sep 27, 2020
This prevents us from sending thousands of notifications to the client
on startup, which can lock up some clients like emacs. Instead we send
precisely one.

This has some consequences for the behaviour of the fast file existence
lookup, which I've noted in the code, alongside a description of how it
works (I spent a while figuring it out, I thought I might as well write
it down).

Fixes #776.
michaelpj referenced this issue in michaelpj/ghcide Sep 27, 2020
This prevents us from sending thousands of notifications to the client
on startup, which can lock up some clients like emacs. Instead we send
precisely one.

This has some consequences for the behaviour of the fast file existence
lookup, which I've noted in the code, alongside a description of how it
works (I spent a while figuring it out, I thought I might as well write
it down).

Fixes #776.
michaelpj referenced this issue in michaelpj/ghcide Sep 27, 2020
This prevents us from sending thousands of notifications to the client
on startup, which can lock up some clients like emacs. Instead we send
precisely one.

This has some consequences for the behaviour of the fast file existence
lookup, which I've noted in the code, alongside a description of how it
works (I spent a while figuring it out, I thought I might as well write
it down).

Fixes #776.
pepeiborra referenced this issue in haskell/ghcide Sep 27, 2020
* FileExists: set one watcher instead of thousands

This prevents us from sending thousands of notifications to the client
on startup, which can lock up some clients like emacs. Instead we send
precisely one.

This has some consequences for the behaviour of the fast file existence
lookup, which I've noted in the code, alongside a description of how it
works (I spent a while figuring it out, I thought I might as well write
it down).

Fixes #776.

* Use fast rules only if it matches our watcher spec
pepeiborra referenced this issue in pepeiborra/ide Dec 29, 2020
* FileExists: set one watcher instead of thousands

This prevents us from sending thousands of notifications to the client
on startup, which can lock up some clients like emacs. Instead we send
precisely one.

This has some consequences for the behaviour of the fast file existence
lookup, which I've noted in the code, alongside a description of how it
works (I spent a while figuring it out, I thought I might as well write
it down).

Fixes haskell/ghcide#776.

* Use fast rules only if it matches our watcher spec
pepeiborra referenced this issue in pepeiborra/ide Dec 29, 2020
* FileExists: set one watcher instead of thousands

This prevents us from sending thousands of notifications to the client
on startup, which can lock up some clients like emacs. Instead we send
precisely one.

This has some consequences for the behaviour of the fast file existence
lookup, which I've noted in the code, alongside a description of how it
works (I spent a while figuring it out, I thought I might as well write
it down).

Fixes haskell/ghcide#776.

* Use fast rules only if it matches our watcher spec
pepeiborra referenced this issue in pepeiborra/ide Dec 29, 2020
* FileExists: set one watcher instead of thousands

This prevents us from sending thousands of notifications to the client
on startup, which can lock up some clients like emacs. Instead we send
precisely one.

This has some consequences for the behaviour of the fast file existence
lookup, which I've noted in the code, alongside a description of how it
works (I spent a while figuring it out, I thought I might as well write
it down).

Fixes haskell/ghcide#776.

* Use fast rules only if it matches our watcher spec
@pepeiborra pepeiborra transferred this issue from haskell/ghcide Jan 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants