bug#70036: a fix that

[Theodor Thornhill]

João Távora <joaotavora@gmail.com> writes:

> On Thu, Apr 18, 2024 at 11:06 PM João Távora <joaotavora@gmail.com> wrote:
>
>> Anyway is this the hotspot I should be trying to optimize?
>>
>> >            9   4%              - find-buffer-visiting
>
> Alright, i've reproduced this
>
>           33   5%       - eglot-handle-notification
>           33   5%        - apply
>           33   5%         - #<compiled-function B79>
>           29   4%          - find-buffer-visiting
>           29   4%           - file-truename
>           29   4%            - file-truename
>           25   4%             - file-truename
>           25   4%              - file-truename
>           25   4%               - file-truename
>           25   4%                - file-truename
>           25   4%                 + file-truename
>
> But I have to say, I wouldn't call this a severe performance penalty.
> I followed your instructions very closely and invoked Emacs like this:
>
> emacs -Q foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/gin/fs.go  -f
> go-ts-mode
>
> The directory is ~/tmp/theo/foo/bar... so it's a pretty long path with
> many directories all in all.

Great!

>
> But I didn't have to wait 10 seconds for the LSP to settle down!  It was
> pretty snappy on my 2018 Lenovo T480 running Archlinux.  And if I
> profile anything other than the initial M-x eglot (which normally happens
> only once in a work session), I don't find any file-truename in the profile.
>

This is true to some extent, but varies a lot from server to server, as
you've also noted earlier. I have an even stronger computer, a 2023 P1
gen 5, I believe, running ubuntu. Some servers send _all_ diagnostics on
every keystroke.

> So my perception is that it must have spent around 4% of 1 second in
> file-truename.
>
> Anyway the reason this shows in this profile is because this project
> with this particular server sends a lot of publishDiagnostics upfront.
> That's OK.  Gopls is a very good server.  I think I see a fix.  But can
> you qualitatively describe the Eglot experience.  Do you notice any
> input lag or something like that? With this project?  I didn't feel _any_
> lag.
> Super snappy.  Maybe the JSON serde kicking in?
>

In this particular project I don't notice any input lag. But on every
java project at work I absolutely do. With my fix I don't. So if we
solve this in any particular way that will be a huge benefit for
everyone using java and other either suboptimal lsp servers or languages
in general.

> Anyway, the idea I suggested earlier is in the patch after  my sig.
>
> Let's think:  LSP's publishDiagnostics coming from the server deals
> in URIs, right?  And we inform the LSP server about URIs, too, right?
> So the URI is always LSP's idea of the resource identifier (and it likes
> to have truename URI).

Sure, like in my key/value store. (apart from symlinking)

>
> My last "better fix" commit records this URI in the buffer as a buffer
> local variable eglot--cached-tdi and it has to do that for every didOpen.
>
> So, to find an open buffer pertaining to  a certain LSP's publishDiagnostics
> it suffices in theory to go through all the buffers that have a non-nil
> cached
> URI and compare that.
>
> No need to convert from URI to file names, not for this job at least!
> I tried this and it worked fine.
>
> When I do that, the profile is completely free of those 4% that
> bothered you.
>
> I'm still testing this for edge cases and will sleep on it, but it seems
> promisingly simple at least.  I can't run unit tests right now, because
> a recent adventurous commit by Stefan Monnier broke them all :-)
> but I'm confident that will be fixed soon...
>
> I hope you can try this patch.

I will :)

>
> João
>
> diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
> index 90a607075d3..38a16b15e4c 100644
> --- a/lisp/progmodes/eglot.el
> +++ b/lisp/progmodes/eglot.el
> @@ -2381,6 +2381,9 @@ eglot-handle-notification
>                          (lambda ()
>                            (remhash token (eglot--progress-reporters
> server))))))))))
>
> +(defvar-local eglot--cached-tdi nil
> +  "A cached LSP TextDocumentIdentifier URI string.")
> +
>  (cl-defmethod eglot-handle-notification
>    (_server (_method (eql textDocument/publishDiagnostics)) &key uri
> diagnostics
>             &allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
> @@ -2391,9 +2394,14 @@ eglot-handle-notification
>                      ((= sev 2)  'eglot-warning)
>                      (t          'eglot-note)))
>              (mess (source code message)
> -              (concat source (and code (format " [%s]" code)) ": "
> message)))
> +              (concat source (and code (format " [%s]" code)) ": "
> message))
> +            (find-it (uri)
> +              (cl-loop for b in (buffer-list)

Could we rather use eglot--managed-buffers, like in my patch? there
shouldn't be a need to loop through say 200 buffers that are unrelated
to the project in question, right? Apart from this I agree, and will try
it.

Thanks,
Theo