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

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

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

ACK.
 
>> 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.

You're right it can't.  A patch that implements without much code
repetition and keeps support for the "push" diagnostics model is
welcome.  Would you like to work on it, JD?

As far as I understand, this model is much more complicated and allows
you to pull diagnostics for individual LSP documents or the whole
project.  One of the difficulties I envison is to do it in a way that
maintains support for project-wide diagnostics.

But its certainly not impossible and would be a wonderful addition,
fixing many problems such as
https://github.com/joaotavora/eglot/issues/1296.

>> 
>> 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.

When a Flymake backend passes on to Flymake a 0-dimensional point in a
file you still want Flymake to create a diagnostic emcompassing a
1-dimensional span of buffer positions.

thingatpt.el is, AFAICT, the standard Emacs's way to move from the 0
dimension to the 1 dimension space.  It needs, quite understandibly,
help from the major mode to do that job.

If that help comes at a dog slow pace, I think that's a problem in
itself.

It takes around 10~20 microsseconds on my machine in Emacs Lisp mode as
measured by:

(/ (car (benchmark-run 10000 (thing-at-point 'sexp))) 10000)

Even less in c++-ts-mode, around 4 microsseconds.

So if python-mode is taking around a hundred thousand times (!) more to
compute that piece of information for some buffer positions, I'd say
it's definitely something to look at...

>From a quick check it seems indeed that the time it takes to compute it
is proportional to the value of point the buffer.  Suggest creating a
bug for python-mode, or calling in whoever wrote that part.
 
>> 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.

Maybe, IF we can confidently say that the server is in the wrong. 

But I added it there for a reason, and quite a long time ago. See the
commit log for

commit 7826b265a0ecd9357719b2fb9491c9bcb517d4cc
Author: João Távora <joaotavora@gmail.com>
Date:   Thu Jun 21 23:32:14 2018 +0100

I'm moderately sure someone somewhere expects the botches ranges to be
auto-corrected by Eglot (though admittedly not at the expense of large
delays).

João