bug#70036: a fix that

["João Távora" <joaotavora@gmail.com>]

On Thu, Apr 18, 2024 at 10:32 PM Theodor Thornhill <theo@thornhill.no> wrote:
>
> Im having some email client issues. I hope this mail renders readable.

> > It doesn't suggest anything other than it, nothing about
> > textDocument/publishDiagnostics.  There's 0 data about that
> > and it never showed up in my profile.
>
> I don't remember exactly right now, but I believe I mentioned many times
> the performance of file-truename directly, and find-buffer-visiting
> which uses file-truename indirectly.

Yes, but that's not the same as saying there's data about it.  Your
profiles don't show find-buffer-visiting as a hotspot.  Or did I miss
any?

> Did I decline? I've lost track, but I believe I said something in the
> lines of "let's discuss it first", but nevermind.

Well, that's declining.  Politely, but declining :)

> > Still, for the very little data that there is available, I do take care
> > to put in a much simpler fix that completely fixes the issue - as far as
> > I or anyone reading the available data can see it.
>
> With this I disagree - but I guess I either have miscommunicated or have
> provided unclear profiles or something inbetween.

I scanned the bug thread twice and couldn't find any other profiles.
There are mentions of textDocument/publishDiagnostics, but no actual
profile data or information of how to see the performance problem.

Maybe _I_ missed something.  But I see you have now sent a perfect
reproduction, so it doesn't matter.

> > And nothing serious will happen after I reverted it, will there?
>
> Not serious, but now we have a performance regression ;-)

As far as we can tell, *you* have one.  Noone else has -- as far as I
can tell from the 7 years that code has been there.  So I wouldn't
really call this a "regression", but that doesn't mean I won't try
to solve it.  I'm confident it can be done. Incidentally I work on
deep repos *with* symlinks, so I'm personally interested in the solution
too.

> I'm fine. I do think you do come off strongly though, and it is not
> always easy to judge if it is of good faith.

Just think: what reason do I have to be in "bad faith"?  I just invited
you to collaborate in the GitHub repo and I want you to keep learning
and improving Eglot.  I just think you did a commit -- in perfectly
good faith -- that clashes directly with what is a tenet of Eglot:
correctness and backward compatibility.  I also think there are other
much better ways to fix the problem (the greatest of these ideas is
yours!!! rewrite it is C).

> And I don't think Eli moralizes on my behalf.

I don't know why it's done and I don't care, but I think it helps noone.

> 1. Install gopls
> 2. Make some directory you can wipe out after the test and cd into it
> 3. git clone git@github.com:theothornhill/gin.git foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/gin
> 4. cd foo/bar/baz/foo/bar/baz/foo/bar/baz/foo/bar/baz/gin
> 5. open fs.go in emacs and make sure some go mode is available. Go-ts-mode for example
> 6. M-x profiler-start
> 7. M-x eglot
> 8. Wait 10-20 seconds. Do no actions other than let the lsp settle.
> 9. M-x profiler-stop
> 10. M-x profiler-report
> 11. Rinse repeat with both or all variants of emacs with/without the
> latest eglot changes.
> I'll add my profiles, and let some metrics talk. This is from emacs -Q
> from your "better fix" and the commit before your revert. Please try on
> your systems and report back.

Great recipe.

> By the way. I have a hunch the reason this is so apparent now is because
> of the faster json serde recently added to emacs. Not sure, but feels
> like it. Json serde is almost gone from the profiles.

What is Json serde?  Is it Json SERialization/DEserialization?
That commit by Hermann made it in? That's great!!  So Eglot is probably
faster overall. So I guess your bug report is about not missing
a further optimization opportunity.  Fine.

Anyway is this the hotspot I should be trying to optimize?

>            9   4%              - find-buffer-visiting

I still think the cleanest solution is to write file-truename
in C.  But if that can't be done, it doesn't seem terribly hard
to get rid of find-buffer-visiting in publishDiagnostics and
still remain symlink-correct.

That's because every interesting result of find-buffer-visiting
is a buffer for which we've already issued a 'didOpen', which
in turn means we've already called file-truename once for it.
If we cache that result (the basic idea behind the "better fix")
it should be possible to find the buffer quicker just by iterating
the list.  That's what I will try to do, using your recipe as
guide.

João