* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) [not found] ` <20240403132719.A18EFC12C28@vcs2.savannah.gnu.org> @ 2024-04-17 15:35 ` João Távora 2024-04-17 18:41 ` Theodor Thornhill 0 siblings, 1 reply; 15+ messages in thread From: João Távora @ 2024-04-17 15:35 UTC (permalink / raw) To: Theodor Thornhill, emacs-devel 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'. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) 2024-04-17 15:35 ` master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) João Távora @ 2024-04-17 18:41 ` Theodor Thornhill 2024-04-18 0:24 ` João Távora 0 siblings, 1 reply; 15+ messages in thread From: Theodor Thornhill @ 2024-04-17 18:41 UTC (permalink / raw) To: João Távora, emacs-devel > 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) 2024-04-17 18:41 ` Theodor Thornhill @ 2024-04-18 0:24 ` João Távora 2024-04-18 5:49 ` Eli Zaretskii 2024-04-18 6:02 ` Theodor Thornhill 0 siblings, 2 replies; 15+ messages in thread From: João Távora @ 2024-04-18 0:24 UTC (permalink / raw) To: Theodor Thornhill; +Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) 2024-04-18 0:24 ` João Távora @ 2024-04-18 5:49 ` Eli Zaretskii 2024-04-18 6:16 ` Theodor Thornhill 2024-04-18 8:34 ` João Távora 2024-04-18 6:02 ` Theodor Thornhill 1 sibling, 2 replies; 15+ messages in thread From: Eli Zaretskii @ 2024-04-18 5:49 UTC (permalink / raw) To: João Távora; +Cc: theo, emacs-devel > 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) 2024-04-18 5:49 ` Eli Zaretskii @ 2024-04-18 6:16 ` Theodor Thornhill 2024-04-18 8:34 ` João Távora 1 sibling, 0 replies; 15+ messages in thread From: Theodor Thornhill @ 2024-04-18 6:16 UTC (permalink / raw) To: Eli Zaretskii, João Távora; +Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) 2024-04-18 5:49 ` Eli Zaretskii 2024-04-18 6:16 ` Theodor Thornhill @ 2024-04-18 8:34 ` João Távora 2024-04-18 9:57 ` Theodor Thornhill 1 sibling, 1 reply; 15+ messages in thread From: João Távora @ 2024-04-18 8:34 UTC (permalink / raw) To: Eli Zaretskii; +Cc: theo, emacs-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) 2024-04-18 8:34 ` João Távora @ 2024-04-18 9:57 ` Theodor Thornhill 2024-04-18 15:00 ` João Távora 0 siblings, 1 reply; 15+ messages in thread From: Theodor Thornhill @ 2024-04-18 9:57 UTC (permalink / raw) To: João Távora, Eli Zaretskii; +Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) 2024-04-18 9:57 ` Theodor Thornhill @ 2024-04-18 15:00 ` João Távora 2024-04-18 15:44 ` Eli Zaretskii 0 siblings, 1 reply; 15+ messages in thread From: João Távora @ 2024-04-18 15:00 UTC (permalink / raw) To: Theodor Thornhill; +Cc: Eli Zaretskii, emacs-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) 2024-04-18 15:00 ` João Távora @ 2024-04-18 15:44 ` Eli Zaretskii 2024-04-18 16:22 ` João Távora 0 siblings, 1 reply; 15+ messages in thread From: Eli Zaretskii @ 2024-04-18 15:44 UTC (permalink / raw) To: João Távora; +Cc: theo, emacs-devel > 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) 2024-04-18 15:44 ` Eli Zaretskii @ 2024-04-18 16:22 ` João Távora 2024-04-18 16:30 ` Theodor Thornhill 2024-04-18 16:35 ` Eli Zaretskii 0 siblings, 2 replies; 15+ messages in thread From: João Távora @ 2024-04-18 16:22 UTC (permalink / raw) To: Eli Zaretskii; +Cc: theo, emacs-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) 2024-04-18 16:22 ` João Távora @ 2024-04-18 16:30 ` Theodor Thornhill 2024-04-18 17:12 ` João Távora 2024-04-18 16:35 ` Eli Zaretskii 1 sibling, 1 reply; 15+ messages in thread From: Theodor Thornhill @ 2024-04-18 16:30 UTC (permalink / raw) To: João Távora; +Cc: Eli Zaretskii, emacs-devel [-- Attachment #1: Type: text/html, Size: 2223 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) 2024-04-18 16:30 ` Theodor Thornhill @ 2024-04-18 17:12 ` João Távora 0 siblings, 0 replies; 15+ messages in thread From: João Távora @ 2024-04-18 17:12 UTC (permalink / raw) To: Theodor Thornhill; +Cc: Eli Zaretskii, emacs-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) 2024-04-18 16:22 ` João Távora 2024-04-18 16:30 ` Theodor Thornhill @ 2024-04-18 16:35 ` Eli Zaretskii 1 sibling, 0 replies; 15+ messages in thread From: Eli Zaretskii @ 2024-04-18 16:35 UTC (permalink / raw) To: João Távora; +Cc: theo, emacs-devel > 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) 2024-04-18 0:24 ` João Távora 2024-04-18 5:49 ` Eli Zaretskii @ 2024-04-18 6:02 ` Theodor Thornhill 2024-04-18 14:49 ` João Távora 1 sibling, 1 reply; 15+ messages in thread From: Theodor Thornhill @ 2024-04-18 6:02 UTC (permalink / raw) To: João Távora; +Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) 2024-04-18 6:02 ` Theodor Thornhill @ 2024-04-18 14:49 ` João Távora 0 siblings, 0 replies; 15+ messages in thread From: João Távora @ 2024-04-18 14:49 UTC (permalink / raw) To: Theodor Thornhill; +Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-04-18 17:12 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <171215083924.12380.5369373861551158668@vcs2.savannah.gnu.org> [not found] ` <20240403132719.A18EFC12C28@vcs2.savannah.gnu.org> 2024-04-17 15:35 ` master 08c80c45dde: Don't use file-truepath in Eglot (bug#70036) João Távora 2024-04-17 18:41 ` Theodor Thornhill 2024-04-18 0:24 ` João Távora 2024-04-18 5:49 ` Eli Zaretskii 2024-04-18 6:16 ` Theodor Thornhill 2024-04-18 8:34 ` João Távora 2024-04-18 9:57 ` Theodor Thornhill 2024-04-18 15:00 ` João Távora 2024-04-18 15:44 ` Eli Zaretskii 2024-04-18 16:22 ` João Távora 2024-04-18 16:30 ` Theodor Thornhill 2024-04-18 17:12 ` João Távora 2024-04-18 16:35 ` Eli Zaretskii 2024-04-18 6:02 ` Theodor Thornhill 2024-04-18 14:49 ` João Távora
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).