Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[João Távora]

Hey Theo,

I don't disagree strongly with these changes (*), but was surprised
to find them pushed without asking me specifically about this patch.
It's true you CCed me about the need for file-truename and I didn't
reply.  But still, another email with the patch about to be pushed
would have been nice.

I have been looking for an Eglot maintainer but everyone I contacted
said no, so I assume I'm still the maintainer.  This is in contrast
to Flymake and Jsonrpc where I've found two people willing to take it
up.

I still get some email and GitHub traffic about Eglot.  I'd very much
like to hand it over to you guys formally, I think you, Felicián,
Stefan, and others will do a fine job, collectively.  Even if I would
prefer to hand it over to someone with some kind of vision for it
(but hey beggars can't be choosers).

I'd also like to be a "whole-package situation", where you guys also
watch over the downstream GitHub repo (or convert it or phase it out
or whatever) so I can unsubscribe in peace.

Just now I got this report in the GitHub tracker about Eglot
spamming *Messages* https://github.com/joaotavora/eglot/discussions/1389
and was caught a bit off-guard, as I had nothing to do with it.

So I propose that as long as you and others (legitimately, of course)
decline the "whole package" offer, you keep giving me just a tiny bit of heads
up about what's being proposed to the code (beside the trivial "add
support for XYZ server" of course).  Or if you prefer just _inform_ me
that this or that change was pushed recently, so I can comment back
while the matter is still fresh.

Felicián did this just today with a patch proposal.

Thanks.  And thanks for your work on Eglot, of course.
João


(*) As to the actual change...

On Wed, Apr 3, 2024 at 2:27 PM Theodor Thornhill via Mailing list for
Emacs changes <emacs-diffs@gnu.org> wrote:

>     the implementation to a hash map will yield similar performance
>     benefits, but wouldn't require us to rewrite `file-truename' in C.


The change seems well structured, well coded, and well described in the
commit message, so I could understand it easily.  Do keep that up.

But of course going from file-buffer-visiting to something else
whose underlying implementation is faster but doesn't chase
symlinks is probably going to have some kind of functional implication
right?  I wonder if (or rather "I hope that") you guys considered it.
Also, when using a cache to solve a problem, remember cache invalidation
is one of the 2 hard ones ;-)  Sure this cache won't ever need
invalidation?  Even when the user moves file around during an Eglot
session?

If you ask me, this 'rewrite `file-truename' in C.' is what should be
done.   No idea how hard that is, but a hash table just isn't fit to answer the
same questions as 'find-buffer-visiting'.

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[Theodor Thornhill]

> Hey Theo,
>

Hey!

> I don't disagree strongly with these changes (*), but was surprised
> to find them pushed without asking me specifically about this patch.
> It's true you CCed me about the need for file-truename and I didn't
> reply.  But still, another email with the patch about to be pushed
> would have been nice.

Sure thing - I'll consider this next time. I did mention it in the bug
report, but could have been more clear, absolutely.

>
> I have been looking for an Eglot maintainer but everyone I contacted
> said no, so I assume I'm still the maintainer.  This is in contrast
> to Flymake and Jsonrpc where I've found two people willing to take it
> up.
>
> I still get some email and GitHub traffic about Eglot.  I'd very much
> like to hand it over to you guys formally, I think you, Felicián,
> Stefan, and others will do a fine job, collectively.  Even if I would
> prefer to hand it over to someone with some kind of vision for it
> (but hey beggars can't be choosers).
>
> I'd also like to be a "whole-package situation", where you guys also
> watch over the downstream GitHub repo (or convert it or phase it out
> or whatever) so I can unsubscribe in peace.

If you want I can do this - so that you don't have to. I've been
considering it for some time after I responded to your call for
maintainers.

>
> Just now I got this report in the GitHub tracker about Eglot
> spamming *Messages* https://github.com/joaotavora/eglot/discussions/1389
> and was caught a bit off-guard, as I had nothing to do with it.
>

;-)

> So I propose that as long as you and others (legitimately, of course)
> decline the "whole package" offer, you keep giving me just a tiny bit of heads
> up about what's being proposed to the code (beside the trivial "add
> support for XYZ server" of course).  Or if you prefer just _inform_ me
> that this or that change was pushed recently, so I can comment back
> while the matter is still fresh.
>
> Felicián did this just today with a patch proposal.
>

Sure

> Thanks.  And thanks for your work on Eglot, of course.
> João
>
>
> (*) As to the actual change...
>
> On Wed, Apr 3, 2024 at 2:27 PM Theodor Thornhill via Mailing list for
> Emacs changes <emacs-diffs@gnu.org> wrote:
>
>>     the implementation to a hash map will yield similar performance
>>     benefits, but wouldn't require us to rewrite `file-truename' in C.
>
>
> The change seems well structured, well coded, and well described in the
> commit message, so I could understand it easily.  Do keep that up.
>
> But of course going from file-buffer-visiting to something else
> whose underlying implementation is faster but doesn't chase
> symlinks is probably going to have some kind of functional implication
> right?  I wonder if (or rather "I hope that") you guys considered it.

I did - and afaict it has no such implications. It

> Also, when using a cache to solve a problem, remember cache invalidation
> is one of the 2 hard ones ;-)  Sure this cache won't ever need
> invalidation?  Even when the user moves file around during an Eglot
> session?

The cache cleans itself up - when a buffer is deleted or the minor mode
shut off. Eglot before this change was basically the same, except it
kept buffers in a list rather than a lookup map. The change is mostly a
complexity improvement, with the added benefit that we don't have to
rely on file-truename.

>
> If you ask me, this 'rewrite `file-truename' in C.' is what should be
> done.   No idea how hard that is, but a hash table just isn't fit to answer the
> same questions as 'find-buffer-visiting'.

That's right, and to some extent I agree. However, find-buffer-visiting
isn't really what's needed in this case, at least from what I can see.
I'm still considering rewriting it in C, but I didn't get to this yet.

Theo

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[João Távora]

On Wed, Apr 17, 2024 at 7:41 PM Theodor Thornhill <theo@thornhill.no> wrote:
have been looking for an Eglot maintainer but everyone I contacted

> If you want I can do this - so that you don't have to. I've been
> considering it for some time after I responded to your call for
> maintainers.

OK, I've added you to the GitHub repo as a collaborator.  Let me know
off-list what your intentions are (to phase it out, keep it and take
it over, or whatever) and let's see how it goes.

> > The change seems well structured, well coded, and well described in the
> > commit message, so I could understand it easily.  Do keep that up.
> >
> > But of course going from file-buffer-visiting to something else
> > whose underlying implementation is faster but doesn't chase
> > symlinks is probably going to have some kind of functional implication
> > right?  I wonder if (or rather "I hope that") you guys considered it.
>
> I did - and afaict it has no such implications. It

I've identified at least one implication.  A rather obvious one, given
that you've effectively removed symlink-awareness from Eglot.

When working in a project with symlinks and visiting such symlinks,
the LSP server  is now informed (via LSP 'didOpen')  of the symlinked
file as if it was its own file, whereas before your changes Eglot
sent over the true name of the file.   You can see this easily
from M-x eglot-events-buffer.

What does this mean in practice?  Many things potentially, but
at least this one: I've run this experiment:  In a new dir, create
a lib.h file with this silly content:

  int foo();

Then create a main.cpp like this:

  #include "lib.h"
  int main() { return foo(); }

Then create a mainlink.cpp symbolic link to main.cpp

Then M-x eglot (launches clangd server to manage the new directory
as a project).

Let's say that during the Eglot session I visit both main.cpp
and mainlink.cpp in different buffers (either because I don't visit
them at the same time or because find-file-existing-other-name is
nil).  Then I press M-? on lib.h's foo() to tell me who references it.

Before you change, Eglot will -- correctly -- tell me there  is a single
user of lib.h's foo() function in my project.

After your change, it tells me there are two users.  This is wrong,
there is only one.

It could be that some servers with direct access to the file system
can deduplicate the information and add back the symlink smarts.

But clangd doesn't do this, and in general servers _can't_ do
this because LSP models a virtual file system.

And for symlinks to large enough files, I'd be surprised if this
doesn't slow down the performance of the server because it has to
analyse what it is told is a completely new file.

So this seems like a pretty big flaw to me after just minimal
surface scratching.  Please reinstate the previous code.

We can resume discussion of your performance problems in the
bug report.  I've seen the profile reports and file-truename is
not thaat big of a problem.  But nevertheless I think I can
help you think of other places to cache things.  Caching
eglot--TextDocumentIdentifier per-buffer seems like a much
easier way to solve your problems.

João

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 18 Apr 2024 01:24:59 +0100
> Cc: emacs-devel@gnu.org
> 
> Let's say that during the Eglot session I visit both main.cpp
> and mainlink.cpp in different buffers (either because I don't visit
> them at the same time or because find-file-existing-other-name is
> nil).  Then I press M-? on lib.h's foo() to tell me who references it.
> 
> Before you change, Eglot will -- correctly -- tell me there  is a single
> user of lib.h's foo() function in my project.
> 
> After your change, it tells me there are two users.  This is wrong,
> there is only one.
> 
> It could be that some servers with direct access to the file system
> can deduplicate the information and add back the symlink smarts.
> 
> But clangd doesn't do this, and in general servers _can't_ do
> this because LSP models a virtual file system.
> 
> And for symlinks to large enough files, I'd be surprised if this
> doesn't slow down the performance of the server because it has to
> analyse what it is told is a completely new file.
> 
> So this seems like a pretty big flaw to me after just minimal
> surface scratching.  Please reinstate the previous code.

I asked exactly this question when the change was discussed, and was
told that symlinks are not a problem.

If we need to support symlinks in Emacs instead of leaving this to the
LSP servers, we could perhaps do that once in some strategic place,
instead of using file-truename everywhere where normally
expand-file-name would do.  Or maybe explicitly test with
file-symlink-p before using file-truename, which is (and has to be)
pretty expensive.  IOW, "punishing" everyone for the benefit of
relatively rare use cases is not the best optimization.

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[Theodor Thornhill]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Date: Thu, 18 Apr 2024 01:24:59 +0100
>> Cc: emacs-devel@gnu.org
>> 
>> Let's say that during the Eglot session I visit both main.cpp
>> and mainlink.cpp in different buffers (either because I don't visit
>> them at the same time or because find-file-existing-other-name is
>> nil).  Then I press M-? on lib.h's foo() to tell me who references it.
>> 
>> Before you change, Eglot will -- correctly -- tell me there  is a single
>> user of lib.h's foo() function in my project.
>> 
>> After your change, it tells me there are two users.  This is wrong,
>> there is only one.
>> 
>> It could be that some servers with direct access to the file system
>> can deduplicate the information and add back the symlink smarts.
>> 
>> But clangd doesn't do this, and in general servers _can't_ do
>> this because LSP models a virtual file system.
>> 
>> And for symlinks to large enough files, I'd be surprised if this
>> doesn't slow down the performance of the server because it has to
>> analyse what it is told is a completely new file.
>> 
>> So this seems like a pretty big flaw to me after just minimal
>> surface scratching.  Please reinstate the previous code.
>
> I asked exactly this question when the change was discussed, and was
> told that symlinks are not a problem.
>

I still believe it is not. The spec [1] states the URI should follow
rfc3986 [2], which states that

```
   Because URIs exist to identify resources, presumably they should be
   considered equivalent when they identify the same resource.  However,
   this definition of equivalence is not of much practical use, as there
   is no way for an implementation to compare two resources unless it
   has full knowledge or control of them.  For this reason,
   determination of equivalence or difference of URIs is based on string
   comparison, perhaps augmented by reference to additional rules
   provided by URI scheme definitions.  We use the terms "different" and
   "equivalent" to describe the possible outcomes of such comparisons,
   but there are many application-dependent versions of equivalence.
```

This to me reads that we shouldn't really consider symlinks, and if
necessary it should be done on the server side. 

I tried the exact same recipe Joao provided in java and go. It works in
java, but not go, and that is mentioned here [3]

> If we need to support symlinks in Emacs instead of leaving this to the
> LSP servers, we could perhaps do that once in some strategic place,
> instead of using file-truename everywhere where normally
> expand-file-name would do.  Or maybe explicitly test with
> file-symlink-p before using file-truename, which is (and has to be)
> pretty expensive.  IOW, "punishing" everyone for the benefit of
> relatively rare use cases is not the best optimization

I agree with this. I'll test it a little.

Theo


[1]: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#uri
[2]: https://datatracker.ietf.org/doc/html/rfc3986
[3]: https://go-review.googlesource.com/c/tools/+/454355

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[João Távora]

On Thu, Apr 18, 2024 at 6:49 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Date: Thu, 18 Apr 2024 01:24:59 +0100
> > Cc: emacs-devel@gnu.org
> >
> > Let's say that during the Eglot session I visit both main.cpp
> > and mainlink.cpp in different buffers (either because I don't visit
> > them at the same time or because find-file-existing-other-name is
> > nil).  Then I press M-? on lib.h's foo() to tell me who references it.
> >
> > Before you change, Eglot will -- correctly -- tell me there  is a single
> > user of lib.h's foo() function in my project.
> >
> > After your change, it tells me there are two users.  This is wrong,
> > there is only one.
> >
> > It could be that some servers with direct access to the file system
> > can deduplicate the information and add back the symlink smarts.
> >
> > But clangd doesn't do this, and in general servers _can't_ do
> > this because LSP models a virtual file system.
> >
> > And for symlinks to large enough files, I'd be surprised if this
> > doesn't slow down the performance of the server because it has to
> > analyse what it is told is a completely new file.
> >
> > So this seems like a pretty big flaw to me after just minimal
> > surface scratching.  Please reinstate the previous code.
>
> I asked exactly this question when the change was discussed, and was
> told that symlinks are not a problem.

Surely not by me, and perhaps whoever told you this wasn't
considering this and other scenarios.  Some funcionality works

> If we need to support symlinks in Emacs instead of leaving this to the
> LSP servers, we could perhaps do that once in some strategic place,
> instead of using file-truename everywhere where normally
> expand-file-name would do.  Or maybe explicitly test with
> file-symlink-p before using file-truename, which is (and has to be)
> pretty expensive.  IOW, "punishing" everyone for the benefit of
> relatively rare use cases is not the best optimization.

As far as I can tell, file-truename is (was) only used "naked"
once or twice,  I think it's the use inside "find-buffer-visiting" which is the
most crucial for the scenarios at hand.  I'll try to see if I can separate
them.

João

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[Theodor Thornhill]

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

> On Thu, Apr 18, 2024 at 6:49 AM Eli Zaretskii <eliz@gnu.org> wrote:
>>
>> > From: João Távora <joaotavora@gmail.com>
>> > Date: Thu, 18 Apr 2024 01:24:59 +0100
>> > Cc: emacs-devel@gnu.org
>> >
>> > Let's say that during the Eglot session I visit both main.cpp
>> > and mainlink.cpp in different buffers (either because I don't visit
>> > them at the same time or because find-file-existing-other-name is
>> > nil).  Then I press M-? on lib.h's foo() to tell me who references it.
>> >
>> > Before you change, Eglot will -- correctly -- tell me there  is a single
>> > user of lib.h's foo() function in my project.
>> >
>> > After your change, it tells me there are two users.  This is wrong,
>> > there is only one.
>> >
>> > It could be that some servers with direct access to the file system
>> > can deduplicate the information and add back the symlink smarts.
>> >
>> > But clangd doesn't do this, and in general servers _can't_ do
>> > this because LSP models a virtual file system.
>> >
>> > And for symlinks to large enough files, I'd be surprised if this
>> > doesn't slow down the performance of the server because it has to
>> > analyse what it is told is a completely new file.
>> >
>> > So this seems like a pretty big flaw to me after just minimal
>> > surface scratching.  Please reinstate the previous code.
>>
>> I asked exactly this question when the change was discussed, and was
>> told that symlinks are not a problem.
>
> Surely not by me, and perhaps whoever told you this wasn't
> considering this and other scenarios.  Some funcionality works
>
>> If we need to support symlinks in Emacs instead of leaving this to the
>> LSP servers, we could perhaps do that once in some strategic place,
>> instead of using file-truename everywhere where normally
>> expand-file-name would do.  Or maybe explicitly test with
>> file-symlink-p before using file-truename, which is (and has to be)
>> pretty expensive.  IOW, "punishing" everyone for the benefit of
>> relatively rare use cases is not the best optimization.
>
> As far as I can tell, file-truename is (was) only used "naked"
> once or twice,  I think it's the use inside "find-buffer-visiting" which is the
> most crucial for the scenarios at hand.  I'll try to see if I can separate
> them.
>
> João

This is correct. The find-buffer-visiting is the most crucial one.

Theo

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[João Távora]

On Thu, Apr 18, 2024 at 10:57 AM Theodor Thornhill <theo@thornhill.no> wrote:

> >> If we need to support symlinks in Emacs instead of leaving this to the
> >> LSP servers, we could perhaps do that once in some strategic place,
> >> instead of using file-truename everywhere where normally
> >> expand-file-name would do.  Or maybe explicitly test with
> >> file-symlink-p before using file-truename, which is (and has to be)
> >> pretty expensive.  IOW, "punishing" everyone for the benefit of
> >> relatively rare use cases is not the best optimization.
> >
> > As far as I can tell, file-truename is (was) only used "naked"
> > once or twice,  I think it's the use inside "find-buffer-visiting" which is the
> > most crucial for the scenarios at hand.  I'll try to see if I can separate
> > them.
> >
> > João
>
> This is correct. The find-buffer-visiting is the most crucial one.

And that one is a longstanding Emacs util function that is already
a good "strategic place" IMO.

So there is only one naked call to file-truename, in eglot--path-to-uri.
"URI" is what the server understands so it's essential that we tell
it the real name of the file visited in the Emacs buffer so as not
to trick it.  It's fairly easy to cache eglot--path-to-uri results
on a case by case basis though.

So to summarize:

* we don't use it "everywhere".  We use it once where it matters.
* the "punishment" isn't really severe and the little there was
  of it has been completely  avoided with my changes.

João

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 18 Apr 2024 16:00:53 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> 
> So to summarize:
> 
> * we don't use it "everywhere".  We use it once where it matters.
> * the "punishment" isn't really severe and the little there was
>   of it has been completely  avoided with my changes.

If the last point is true, it should be confirmed by timing the old
and the new code.

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[João Távora]

On Thu, Apr 18, 2024 at 4:44 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Date: Thu, 18 Apr 2024 16:00:53 +0100
> > Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> >
> > So to summarize:
> >
> > * we don't use it "everywhere".  We use it once where it matters.
> > * the "punishment" isn't really severe and the little there was
> >   of it has been completely  avoided with my changes.
>
> If the last point is true, it should be confirmed by timing the old
> and the new code.

It is true at least from what I could gather are Theodor's
experiments.  Theodor states publishDiagnostics is a hotspot, but
I couldn't find it in his files emacs-29-before-everything and
emacs-30-before.  The hotspots I could recognize from that data
are 100% solved by my patch.  The appeared _faintly_ in my tests
(hence the comment about the "severity" of the punishment)
and after the patch they don't appear at all.

I've asked Theodor to post more data and details about a
reproducible experiment.  We should start there.

I apologize in part for not being present in the beginning when
analysing these (one-off and uncommon) performance concerns  but I
don't think that's a reason for rushing a regression-inducing change.

João

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[Theodor Thornhill]

[-- Attachment #1: Type: text/html, Size: 2223 bytes --]

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[João Távora]

On Thu, Apr 18, 2024 at 5:30 PM Theodor Thornhill <theo@thornhill.no> wrote:

> They are not one-off and uncommon - there is no reason to be harsh and unkind.

I'm not being harsh.  Your report is really the first time I ever heard
about this particular problem and there is more than a thousand issues in
the Eglot tracker, and I analyzed all of them (even if briefly).
So it's literally one-off and it's uncommon become one in thousands counts
as uncommon to me.

But that doesn't mean I'm not invested in solving it, now that you've
told me how serious a problem it is for you.  I just don't want to do it
at the cost of breakage to other users who AFAICT have NOT complained
about this (me included).

> I glanced on the lsp-mode repo issues, and comments on file-truepath
> and it's performance killing slowness is abundant.

I don't track lsp-mode repo, as you may understand.  I barely have time
for Eglot.

> I'll create a  reproducible recipe later tonight.

Great idea!

João

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 18 Apr 2024 17:22:12 +0100
> Cc: theo@thornhill.no, emacs-devel@gnu.org
> 
> I apologize in part for not being present in the beginning when
> analysing these (one-off and uncommon) performance concerns  but I
> don't think that's a reason for rushing a regression-inducing change.

The change was not rushed, it was seriously discussed by several
people.

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[Theodor Thornhill]

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

> On Wed, Apr 17, 2024 at 7:41 PM Theodor Thornhill <theo@thornhill.no> wrote:
> have been looking for an Eglot maintainer but everyone I contacted
>
>> If you want I can do this - so that you don't have to. I've been
>> considering it for some time after I responded to your call for
>> maintainers.
>
> OK, I've added you to the GitHub repo as a collaborator.  Let me know
> off-list what your intentions are (to phase it out, keep it and take
> it over, or whatever) and let's see how it goes.
>

Thanks, will do.

>> > The change seems well structured, well coded, and well described in the
>> > commit message, so I could understand it easily.  Do keep that up.
>> >
>> > But of course going from file-buffer-visiting to something else
>> > whose underlying implementation is faster but doesn't chase
>> > symlinks is probably going to have some kind of functional implication
>> > right?  I wonder if (or rather "I hope that") you guys considered it.
>>
>> I did - and afaict it has no such implications. It
>
> I've identified at least one implication.  A rather obvious one, given
> that you've effectively removed symlink-awareness from Eglot.
>
> When working in a project with symlinks and visiting such symlinks,
> the LSP server  is now informed (via LSP 'didOpen')  of the symlinked
> file as if it was its own file, whereas before your changes Eglot
> sent over the true name of the file.   You can see this easily
> from M-x eglot-events-buffer.
>
> What does this mean in practice?  Many things potentially, but
> at least this one: I've run this experiment:  In a new dir, create
> a lib.h file with this silly content:
>
>   int foo();
>
> Then create a main.cpp like this:
>
>   #include "lib.h"
>   int main() { return foo(); }
>
> Then create a mainlink.cpp symbolic link to main.cpp
>
> Then M-x eglot (launches clangd server to manage the new directory
> as a project).
>
> Let's say that during the Eglot session I visit both main.cpp
> and mainlink.cpp in different buffers (either because I don't visit
> them at the same time or because find-file-existing-other-name is
> nil).  Then I press M-? on lib.h's foo() to tell me who references it.
>
> Before you change, Eglot will -- correctly -- tell me there  is a single
> user of lib.h's foo() function in my project.
>
> After your change, it tells me there are two users.  This is wrong,
> there is only one.
>
> It could be that some servers with direct access to the file system
> can deduplicate the information and add back the symlink smarts.
>
> But clangd doesn't do this, and in general servers _can't_ do
> this because LSP models a virtual file system.

From what I can see in other servers this is recognized as a server
problem, and not a client problem [1], [2], [3]. 

>
> And for symlinks to large enough files, I'd be surprised if this
> doesn't slow down the performance of the server because it has to
> analyse what it is told is a completely new file.
>
> So this seems like a pretty big flaw to me after just minimal
> surface scratching.  Please reinstate the previous code.
>

I can absolutely do this, but I'd rather we consider it a bit more
before I do. On my system there is a real, tangible performance increase
with this change, and the symlink one seems more like a server edge case
issue. Maybe we could check for file-symlink-p rather than runthe whole
file-truepath?


> We can resume discussion of your performance problems in the
> bug report.  I've seen the profile reports and file-truename is
> not thaat big of a problem.  But nevertheless I think I can
> help you think of other places to cache things.  Caching
> eglot--TextDocumentIdentifier per-buffer seems like a much
> easier way to solve your problems.

I disagree - it really makes a huge difference in felt latency.

Theo


[1]: https://github.com/clangd/clangd/issues/124
[2]: https://github.com/microsoft/python-language-server/issues/181
[3]: https://github.com/microsoft/vscode-python/issues/2613

Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036)

[João Távora]

On Thu, Apr 18, 2024 at 7:02 AM Theodor Thornhill <theo@thornhill.no> wrote:

> > But clangd doesn't do this, and in general servers _can't_ do
> > this because LSP models a virtual file system.
>
> From what I can see in other servers this is recognized as a server
> problem, and not a client problem [1], [2], [3].

From what I can see it was _reported_ as a server problem.  Whether
that actually means it is _recognized_ as a server problem would
need a much larger consensus between the numerous servers.  There
are very many LSP servers, and new ones pop up every day. It
won't be long until Eglot supports 100 servers.

So even a newfound LSP directive stating this is indeed the server's
responsibility, it wouldn't really be satisfactory, because we're dealing
with a "de facto" spec.

Also the LSP maintainers tend to err on the side of bothering the clients
to do these things.  Sesee the "DidChangeWatchedFiles" notification discussion
in the spec.

They argue that it's easier to do in the client partially because
LSP clients are less numerous than LSP servers.

Myself, I can't think how this can be a server problem.  It makes no
sense to me for a client such as Emacs which has a perfectly sane and
clear view of what is a symlink or not (it tells you explicitly in
the echo eare) to mention the same object twice via its alias.

As I wrote, it may be that some servers have the ability to understand
symlinks because they are close to the file system.  But that's not
necessarily the case.  LSP servers can function in completely
different hosts and be informed of files just via DidOpen, didChange,
DidSave.

Whatever the outcome of that discussion I think we should have
no inclination to expose Eglot users to a new problem.  Also note
that the reports you link to are about looping.  My report is not
about looping at all, it's about incorrect responses.

Taking all this in consideration I have reverted the patch.
I've added a much simpler fix to your performance problems that
doesn't endanger the correct behaviour.

> > And for symlinks to large enough files, I'd be surprised if this
> > doesn't slow down the performance of the server because it has to
> > analyse what it is told is a completely new file.
> >
> > So this seems like a pretty big flaw to me after just minimal
> > surface scratching.  Please reinstate the previous code.
> >
>
> I can absolutely do this, but I'd rather we consider it a bit more
> before I do. On my system there is a real, tangible performance increase
> with this change, and the symlink one seems more like a server edge case
> issue.

Maybe, but bugs about edge cases aren't uncommon or less important IMO.


> Maybe we could check for file-symlink-p rather than runthe whole
> file-truepath?

Maybe, but let's do that after fixing the regression.  Let's follow up
in the bug report, which I've hopefully catched up on.

João