Subject: bug#77588: closed (Re: bug#77588: Catastrophic slowdown in eglot via `flymake-diag-region')

From: [email-masked] (GNU bug Tracking System)
Date: Tue, 08 Apr 2025 15:43:02 +0000

Your bug report

#77588: Catastrophic slowdown in eglot via `flymake-diag-region'

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
77588: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=77588
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#77588: Catastrophic slowdown in eglot via `flymake-diag-region'
From: João Távora <[email-masked]>
To: JD Smith <[email-masked]>
Cc: Spencer Baugh <[email-masked]>, Eli Zaretskii <[email-masked]>, [email-masked], [email-masked]
Date: Tue, 8 Apr 2025 16:42:15 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

On Tue, Apr 8, 2025, 14:46 JD Smith <[email-masked]> wrote:.

I meant LSP version identifiers.  After my success solving the current
> problem using them, I believe ensuring the document version the LSP server
> worked against when working up a response to the diagnostic pull request
> matches the current version is vitally important.  Luckily this looks to be
> straightforward.
>

Yes.

I'll take it under consideration, though I suspect it would be 10x easier
> for someone more familiar with eglot's semantics and code conventions,
> which are more complex than typical packages.
>

The main difficulty seems to be finding a server which supports this mode.
Clangd and basedpyright don't seem to.

>
> Thanks.  This tests fine on master.  Performance in my large python buffer
> is now refreshingly acceptable.  Two questions:
>
> - Can we be certain that `eglot--versioned-identifier' is always an
> integer?
>

No. It can be nil. But what it's not nil, it's an integer as specified in
the standard.

- Any reason not to implement this on the emacs-30 branch?

Eglot is a GNU Elpa :core package and so it has its own release rhythm.

I don't think the LSP server is to blame here.  Yes, it's sending large
> messages (too often) and getting behind, but it is dutifully providing the
> document version number.

I beg to differ. A server that is sending useless junk down the wire,
however legal that junk is, is a server that should be fixed. As you well
know, performance problems arise often from just parsing JSON into
expensive Lisp structures.

João

Subject: Catastrophic slowdown in eglot via `flymake-diag-region'
From: JD Smith <[email-masked]>
To: [email-masked]
Date: Sun, 6 Apr 2025 18:50:26 -0400
List-Id: <debbugs-submit.debbugs.gnu.org>

I was diagnosing a painful eglot pause of about 10 seconds that would reliably occur every few edits when completing candidates and working in a long python file (~8k lines).  I am using the basedpyright LSP server.  I narrowed this down to eglot's `textDocument/publishDiagnostics' notification handler.

The file I was working on has several hundred warnings and errors, so that represents a substantial message from the LSP server.  After a fair bit of timing and sleuthing, I narrowed it down further.  A few interrelated misbehaviors combined to produce the huge intermittent pauses:

When completing text directly after entering/removing a newline, the Diagnostics ranges received by eglot are out of date and "one line off".
Some of these off-by-one-line diagnostic message ranges now land on blank lines in the file, so the effective range there is empty.
Seeing an empty range, eglot declares the server has "botched" it, so it tries to make a by-hand recalculation of the range using `flymake-diag-region'.
`flymake-diag-region' for some reason calls (end-of-thing 'sexp) and (end-of-thing 'symbol).  
In some positions in a long python file these commands are VERY SLOW.  Rather than the typical call time of ~1ms, at these certain positions `flymake-diag-region' takes ~400ms.

It doesn't take too many "botched eglot ranges" interacting with slow `thingatpt' misbehavior to add up to a 10s delay.

I'm not certain of the best solution.  A few ideas, from hardest to easiest:

Teach eglot textDocument/diagnostic
++++++++++++++++++++++++++++

 textDocument/publishDiagnostics messages arrive way too frequently IMO, after every buffer change of any kind.  They are pushed to eglot from the LSP server, and if they contain hundreds of errors, this becomes very inefficient (re-painting with flymake the same hundreds of regions over and over after each keystroke).

The best solution here would probably be to adopt "pull" diagnostics using textDocument/diagnostic, perhaps in an idle-timer whose duration the user can configure.  I don't believe EGLOT can do diagnostic pulls at the moment.

Don't use thingatpt in `flymake-diag-region'
+++++++++++++++++++++++++++++++++

`flymake-diag-region' should perhaps not use thingapt, which is subject to the performance vagaries of the major-mode and underlying file.  I am uncertain why it relies on that.  Perhaps the performance of those will be improved with treesitter variants.

Eglot could detect off-by-one diagnostics
+++++++++++++++++++++++++++++++

Hard to know the best heuristic, but lots of null effective ranges is a good one.

Eglot can simply ignore null range diagnostics
+++++++++++++++++++++++++++++++++++

Eglot doesn't need to use `flymake-diag-region' to try to calculate an update range if it encounters a null diagnostic range.  It could simply drop those, as they are probably wrong anyway, and will shortly be updated.