bug#70036: 30.0.50; Move file-truename to the C level

[Theodor Thornhill via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 3007 bytes --]

Felician Nemeth <felician.nemeth@gmail.com> writes:

> Theodor Thornhill <theo@thornhill.no> writes:
>> Felician Nemeth <felician.nemeth@gmail.com> writes:
>
>>> Theo, can you email me the relevant messages that your server sends
>>> to Emacs?  Does the server send lots of similar diagnostics messages
>>> frequently?
>
>> I'll try to include such a report a little later today. 
>
> Thanks, that would be helpful.
>

It's a little hard scraping the logs for company related data, so I
haven't done this yet, but the profiles should be pretty expanatory if
you do some edits after applying the below patch.

>> [2. text/x-diff; 0001-Don-t-use-file-truepath-in-Eglot-bug-70036.patch]...
>
> I think using find-buffer-visiting instead of get-file-buffer and
> file-truename instead of expand-file-name in Eglot is problematic.
> Let's say we have these files:
>
> /project/a.c
> /project/a.h -> /other/a.h
>
> Eglot will communicate these file names to the LSP server: /project/a.c
> and /other/a.h.  Then the server cannot "associate" a.h with a.c.
> Additionally, a.h will be outside of the LSP workspace.
>
> This indeed confuses clangd a bit: it only takes into account the
> changes of buffer a.h when it is saved.  (Because it assumes
> /project/a.h is not opened in the editor.)

So in other words this is already a bug in eglot? 

>
> ------
>
> Regarding the patch itself, cache invalidation is missing from it.  The
> user might kill a buffer or save it under a different name with
> write-file.  Changing (PATH -> BUFFER) to (PATH -> (BUFFER, FILENAME))
> would probably work.  Eglot should save the current buffer-file-name
> when it inserts a new item into the hash of managed-buffers.  And when
> it retrieves an item, it should verify whether the buffer-file-name is
> the same as the saved file-name.
>
> Can file-truepath change while buffer-file-name remains the same?  Yes,
> but I think Eglot could ignore those rare cases, or handle it elsewhere.
> (For example, it could update the cache entry after a buffer is saved.)

Actually, cache invalidation is there, at least for killing a
buffer. Major modes are disabled on killing buffer, and it is removed as
a part of the teardown. Saving to a new buffer isn't handled properly
yet, but this looks like something not really supported by Eglot yet
anyway. If I resave a buffer with C-x C-w its content will be placed in
a new buffer, but old one will not be deleted, and the new one will not
be registered with the eglot server before running M-x revert-buffer
anyways. So that seems like a different issue, really.

Please check out the new patch. I find no issues diverging from the
current behavior using this one. It seems to solve all the performance
issues I stated, and now completely new stuff shows up in the profile,
which to me sounds like the bottlenecks have moved, suggesting we get a
nice performance upgrade.

What do you think? I'll test this for a while and install to master in a
few days if nothing comes up :-)

Theo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-use-file-truepath-in-Eglot-bug-70036.patch --]
[-- Type: text/x-diff, Size: 8181 bytes --]

From 26e5f3cc8e767215f7c50800a6d713702b8fa144 Mon Sep 17 00:00:00 2001
From: Theodor Thornhill <theo@thornhill.no>
Date: Thu, 28 Mar 2024 12:56:23 +0100
Subject: [PATCH] Don't use file-truepath in Eglot (bug#70036)

`file-truepath' is slow because of recursive calls and being implemented
in lisp.  It seems to not be needed in eglot, but it is used behind the
scenes in `find-buffer-visiting', thus appearing in profiles.  Moving
the implementation to a hash map will yield similar performance
benefits, but wouldn't require us to rewrite `file-truename' in C.

* lisp/progmodes/eglot.el (eglot-lsp-server): Convert 'managed-buffers'
to a hashmap.
(eglot-uri-to-path): Don't use file-truepath, as it is too slow to be
included in the hot path.
(eglot--on-shutdown): Use buffers from buffer map.
(eglot--managed-mode): Add buffer to map, rather than list. Also remove
it from the map on deactivation.
(eglot-handle-notification): Expose server and get buffer from the
buffer map.
---
 lisp/progmodes/eglot.el | 42 +++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index f247c43203c..7f4284bf09d 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -1053,8 +1053,8 @@ eglot-lsp-server
     :documentation "Map (DIR -> (WATCH ID1 ID2...)) for `didChangeWatchedFiles'."
     :initform (make-hash-table :test #'equal) :accessor eglot--file-watches)
    (managed-buffers
-    :initform nil
-    :documentation "List of buffers managed by server."
+    :documentation "Map (PATH -> BUFFER) for buffers managed by server."
+    :initform (make-hash-table :test #'equal)
     :accessor eglot--managed-buffers)
    (saved-initargs
     :documentation "Saved initargs for reconnection purposes."
@@ -1085,12 +1085,12 @@ eglot-uri-to-path
 
 (defun eglot-path-to-uri (path)
   "Convert PATH, a file name, to LSP URI string and return it."
-  (let ((truepath (file-truename path)))
+  (let ((expanded-path (expand-file-name path)))
     (if (and (url-type (url-generic-parse-url path))
              ;; It might be MS Windows path which includes a drive
              ;; letter that looks like a URL scheme (bug#59338)
              (not (and (eq system-type 'windows-nt)
-                       (file-name-absolute-p truepath))))
+                       (file-name-absolute-p expanded-path))))
         ;; Path is already a URI, so forward it to the LSP server
         ;; untouched.  The server should be able to handle it, since
         ;; it provided this URI to clients in the first place.
@@ -1098,11 +1098,11 @@ eglot-path-to-uri
       (concat "file://"
               ;; Add a leading "/" for local MS Windows-style paths.
               (if (and (eq system-type 'windows-nt)
-                       (not (file-remote-p truepath)))
+                       (not (file-remote-p expanded-path)))
                   "/")
               (url-hexify-string
                ;; Again watch out for trampy paths.
-               (directory-file-name (file-local-name truepath))
+               (directory-file-name (file-local-name expanded-path))
                eglot--uri-path-allowed-chars)))))
 
 (defun eglot-range-region (range &optional markers)
@@ -1187,7 +1187,7 @@ eglot--servers-by-xrefed-file
 (defun eglot--on-shutdown (server)
   "Called by jsonrpc.el when SERVER is already dead."
   ;; Turn off `eglot--managed-mode' where appropriate.
-  (dolist (buffer (eglot--managed-buffers server))
+  (dolist (buffer (map-values (eglot--managed-buffers server)))
     (let (;; Avoid duplicate shutdowns (github#389)
           (eglot-autoshutdown nil))
       (eglot--when-live-buffer buffer (eglot--managed-mode-off))))
@@ -1992,7 +1992,11 @@ eglot--managed-mode
       (add-hook 'eldoc-documentation-functions #'eglot-signature-eldoc-function
                 nil t)
       (eldoc-mode 1))
-    (cl-pushnew (current-buffer) (eglot--managed-buffers (eglot-current-server))))
+
+    (let ((buffer (current-buffer)))
+      (puthash (expand-file-name (buffer-file-name buffer))
+               buffer
+               (eglot--managed-buffers (eglot-current-server)))))
    (t
     (remove-hook 'after-change-functions #'eglot--after-change t)
     (remove-hook 'before-change-functions #'eglot--before-change t)
@@ -2020,10 +2024,10 @@ eglot--managed-mode
     (let ((server eglot--cached-server))
       (setq eglot--cached-server nil)
       (when server
-        (setf (eglot--managed-buffers server)
-              (delq (current-buffer) (eglot--managed-buffers server)))
+        (remhash (expand-file-name (buffer-file-name (current-buffer)))
+                 (eglot--managed-buffers server))
         (when (and eglot-autoshutdown
-                   (null (eglot--managed-buffers server)))
+                   (null (map-values (eglot--managed-buffers server))))
           (eglot-shutdown server)))))))
 
 (defun eglot--managed-mode-off ()
@@ -2346,7 +2350,7 @@ eglot-handle-notification
                           (remhash token (eglot--progress-reporters server))))))))))
 
 (cl-defmethod eglot-handle-notification
-  (_server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
+  (server (_method (eql textDocument/publishDiagnostics)) &key uri diagnostics
            &allow-other-keys) ; FIXME: doesn't respect `eglot-strict-mode'
   "Handle notification publishDiagnostics."
   (cl-flet ((eglot--diag-type (sev)
@@ -2357,7 +2361,7 @@ eglot-handle-notification
             (mess (source code message)
               (concat source (and code (format " [%s]" code)) ": " message)))
     (if-let* ((path (expand-file-name (eglot-uri-to-path uri)))
-              (buffer (find-buffer-visiting path)))
+              (buffer (gethash path (eglot--managed-buffers server))))
         (with-current-buffer buffer
           (cl-loop
            initially
@@ -2842,7 +2846,7 @@ eglot--xref-make-match
 Try to visit the target file for a richer summary line."
   (pcase-let*
       ((file (eglot-uri-to-path uri))
-       (visiting (or (find-buffer-visiting file)
+       (visiting (or (gethash file (eglot--managed-buffers (eglot-current-server)))
                      (gethash uri eglot--temp-location-buffers)))
        (collect (lambda ()
                   (eglot--widening
@@ -3542,13 +3546,14 @@ eglot--propose-changes-as-diff
   (with-current-buffer (get-buffer-create "*EGLOT proposed server changes*")
     (buffer-disable-undo (current-buffer))
     (let ((inhibit-read-only t)
-          (target (current-buffer)))
+          (target (current-buffer))
+          (managed-buffers (eglot--managed-buffers (eglot-current-server))))
       (diff-mode)
       (erase-buffer)
       (pcase-dolist (`(,path ,edits ,_) prepared)
         (with-temp-buffer
           (let* ((diff (current-buffer))
-                 (existing-buf (find-buffer-visiting path))
+                 (existing-buf (gethash path (gethash path managed-buffers)))
                  (existing-buf-label (prin1-to-string existing-buf)))
             (with-temp-buffer
               (if existing-buf
@@ -3583,7 +3588,8 @@ eglot--apply-workspace-edit
                      (eglot--dbind ((VersionedTextDocumentIdentifier) uri version)
                          textDocument
                        (list (eglot-uri-to-path uri) edits version)))
-                   documentChanges)))
+                   documentChanges))
+          (managed-buffers (eglot--managed-buffers (eglot-current-server))))
       (unless (and changes documentChanges)
         ;; We don't want double edits, and some servers send both
         ;; changes and documentChanges.  This unless ensures that we
@@ -3591,7 +3597,7 @@ eglot--apply-workspace-edit
         (cl-loop for (uri edits) on changes by #'cddr
                  do (push (list (eglot-uri-to-path uri) edits) prepared)))
       (cl-flet ((notevery-visited-p ()
-                  (cl-notevery #'find-buffer-visiting
+                  (cl-notevery (lambda (p) (gethash p managed-buffers))
                                (mapcar #'car prepared)))
                 (accept-p ()
                   (y-or-n-p
-- 
2.40.1