Re: master 3e4d4f472d: Rework `abbreviate-file-name' in Tramp

Re: master 3e4d4f472d: Rework `abbreviate-file-name' in Tramp

[Jim Porter]

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

On 3/7/2022 4:42 AM, Michael Albinus wrote:
> branch: master
> commit 3e4d4f472d3960a7d18dad76b8d54a66bc5d9f6c
[snip]
> diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
> index 932dfb3691..5bf6a54020 100644
> --- a/lisp/net/tramp.el
> +++ b/lisp/net/tramp.el
[snip]
> @@ -3360,15 +3362,16 @@ Let-bind it when necessary.")
[snip]
>               ;; Otherwise, just use the cached value.
> -            (tramp-get-connection-property vec "home-directory" nil))))
> +            (tramp-get-connection-property vec "~" nil))))
> +    (when home-dir
> +      (setq home-dir
> +	    (tramp-compat-funcall
> +	     'directory-abbrev-apply
> +	     (tramp-make-tramp-file-name vec home-dir))))
>       ;; If any elt of `directory-abbrev-alist' matches this name,
>       ;; abbreviate accordingly.

I don't know how much this matters in practice, but one benefit of the 
old code is that it cached the *abbreviated* home directory (though I 
gave it a poor name; "home-directory" didn't indicate that it's 
abbreviated). This can significantly improve performance when there are 
many entries in `directory-abbrev-alist', since then Tramp doesn't have 
to abbreviate the home-dir on each call. That's similar to how 
`abbreviate-file-name' works on local files, which also caches the 
abbreviated home dir.

See the attached benchmark/results. I also attached a sketch of a patch 
for a way to avoid that performance cost. I'm not sure it's actually a 
good way to do things (or even if it's correct in all cases), but 
hopefully it serves as a demonstration. It would probably be better to 
cache it somewhere that doesn't persist across sessions though[1]; it 
could get out of sync with `directory-abbrev-alist' and cause problems 
(this was a problem with the old code too).

That said, I wouldn't have a problem if the answer is 
"`directory-abbrev-alist' is short, so caching this value is more 
trouble than it's worth." The `abbreviate-file-name' function even asks 
this question (for local files):

       ;; FIXME Is it even worth caching abbreviated-home-dir?
       ;; Ref: https://debbugs.gnu.org/19657#20

- Jim

[1] At least, I *think* Tramp connection properties are cached across 
Emacs sessions...

[-- Attachment #2: benchmark.el --]
[-- Type: text/plain, Size: 1362 bytes --]

(setq tramp-verbose 0
      user-name "jim"
      remote-host (concat "/sshx:" user-name "@localhost:"))

(defun fill-directory-abbrev-alist (count)
  (setq directory-abbrev-alist
        (let (result)
          (dotimes (i count result)
            (setq result (cons (cons (format "\\`/home/abbr%d" (1+ i))
                                     (format "/home/abbr%d" i))
                               result))))))

(defun run-test (count &optional path)
  (let* ((abbreviate-home-dir nil)
         (path (or path (concat "/home/" user-name "/src/project")))
         (remote-path (concat remote-host path)))
    (garbage-collect)
    (benchmark 1000 `(abbreviate-file-name ,path))
    (garbage-collect)
    (benchmark 1000 `(abbreviate-file-name ,remote-path))))

(find-file (concat remote-host "~"))

(message "Empty `directory-abbrev-alist'")
(run-test 1000)
(message "")

(fill-directory-abbrev-alist 100)
(message "100 items in `directory-abbrev-alist' (no matches)")
(run-test 1000)
(message "")

(message "100 items in `directory-abbrev-alist' (all matches)")
(run-test 1000 "/home/abbr100/src/project")
(message "")

(fill-directory-abbrev-alist 500)
(message "500 items in `directory-abbrev-alist' (no matches)")
(run-test 1000)
(message "")

(message "500 items in `directory-abbrev-alist' (all matches)")
(run-test 1000 "/home/abbr100/src/project")

[-- Attachment #3: results.txt --]
[-- Type: text/plain, Size: 1630 bytes --]

Before
------

Empty ‘directory-abbrev-alist’
Local:  Elapsed time: 0.073266s (0.015766s in 1 GCs)
Remote: Elapsed time: 0.660868s (0.260615s in 17 GCs)

100 items in ‘directory-abbrev-alist’ (no matches)
Local:  Elapsed time: 0.347025s (0.147198s in 10 GCs)
Remote: Elapsed time: 0.911555s (0.370291s in 25 GCs)

100 items in ‘directory-abbrev-alist’ (all matches)
Local:  Elapsed time: 0.575803s (0.374109s in 25 GCs)
Remote: Elapsed time: 0.960296s (0.396087s in 26 GCs)

500 items in ‘directory-abbrev-alist’ (no matches)
Local:  Elapsed time: 1.151418s (0.582457s in 38 GCs)
Remote: Elapsed time: 1.716861s (0.802557s in 53 GCs)

500 items in ‘directory-abbrev-alist’ (all matches)
Local:  Elapsed time: 1.396938s (0.815721s in 53 GCs)
Remote: Elapsed time: 1.752481s (0.802967s in 54 GCs)


After
-----

Empty ‘directory-abbrev-alist’
Local:  Elapsed time: 0.073033s (0.014667s in 1 GCs)
Remote: Elapsed time: 0.681975s (0.255890s in 17 GCs)

100 items in ‘directory-abbrev-alist’ (no matches)
Local:  Elapsed time: 0.362925s (0.154917s in 10 GCs)
Remote: Elapsed time: 1.123233s (0.477880s in 32 GCs)

100 items in ‘directory-abbrev-alist’ (all matches)
Local:  Elapsed time: 0.593666s (0.388942s in 25 GCs)
Remote: Elapsed time: 1.143706s (0.484284s in 33 GCs)

500 items in ‘directory-abbrev-alist’ (no matches)
Local:  Elapsed time: 1.121195s (0.561525s in 38 GCs)
Remote: Elapsed time: 2.649969s (1.295120s in 88 GCs)

500 items in ‘directory-abbrev-alist’ (all matches)
Local:  Elapsed time: 1.356323s (0.783774s in 53 GCs)
Remote: Elapsed time: 2.742058s (1.334358s in 89 GCs)

[-- Attachment #4: 0001-Cache-abbreviated-home-directory-for-Tramp.patch --]
[-- Type: text/plain, Size: 2093 bytes --]

From 7602b225487da39ccc639261f426844af1d52ec7 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 7 Mar 2022 22:01:11 -0800
Subject: [PATCH] Cache abbreviated home directory for Tramp

This improves performance when there are many entries in
'directory-abbrev-alist'.

* lisp/net/tramp.el (tramp-handle-abbreviate-file-name): Cache
abbreviated home directory.
---
 lisp/net/tramp.el | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index 5bf6a54020..560ed1f8ed 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -3361,17 +3361,19 @@ tramp-handle-abbreviate-file-name
 	 (vec (tramp-dissect-file-name filename))
 	 (tramp-tolerate-tilde t)
          (home-dir
-          (if (let ((non-essential t)) (tramp-connectable-p vec))
-              ;; If a connection has already been established, get the
-              ;; home directory.
-	      (tramp-get-home-directory vec)
-            ;; Otherwise, just use the cached value.
-            (tramp-get-connection-property vec "~" nil))))
-    (when home-dir
-      (setq home-dir
-	    (tramp-compat-funcall
-	     'directory-abbrev-apply
-	     (tramp-make-tramp-file-name vec home-dir))))
+          (catch 'no-home-dir
+            (with-tramp-connection-property vec "abbr-home-directory"
+	      (tramp-compat-funcall
+	       'directory-abbrev-apply
+               (tramp-make-tramp-file-name
+                vec
+                (if (let ((non-essential t)) (tramp-connectable-p vec))
+                    ;; If a connection has already been established, get the
+                    ;; home directory.
+                    (tramp-get-home-directory vec)
+                  ;; Otherwise, just use the cached value.
+                  (or (tramp-get-connection-property vec "~" nil)
+                      (throw 'no-home-dir)))))))))
     ;; If any elt of `directory-abbrev-alist' matches this name,
     ;; abbreviate accordingly.
     (setq filename (tramp-compat-funcall 'directory-abbrev-apply filename))
-- 
2.25.1

Re: master 3e4d4f472d: Rework `abbreviate-file-name' in Tramp

[Michael Albinus]

Jim Porter <jporterbugs@gmail.com> writes:

Hi Jim,

> I don't know how much this matters in practice, but one benefit of the
> old code is that it cached the *abbreviated* home directory (though I
> gave it a poor name; "home-directory" didn't indicate that it's
> abbreviated). This can significantly improve performance when there
> are many entries in `directory-abbrev-alist', since then Tramp doesn't
> have to abbreviate the home-dir on each call. That's similar to how
> `abbreviate-file-name' works on local files, which also caches the
> abbreviated home dir.

I don't know how relevant it is in practice, but if you believe it is
relevant I don't object to apply such a patch. The reason for my rework
was to fold out handling of home directory from the different
expand-file-name implementations, it didn't work in different backends
(tramp-gvfs.el, tramp-sudoedit.el), or it wasn't implemented at all
(tramp-smb.el). So I've introduced the new internal magic file name
operation tramp-get-home-directory, which has different implementations
in different backends.

> See the attached benchmark/results. I also attached a sketch of a
> patch for a way to avoid that performance cost. I'm not sure it's
> actually a good way to do things (or even if it's correct in all
> cases), but hopefully it serves as a demonstration. It would probably
> be better to cache it somewhere that doesn't persist across sessions
> though[1]; it could get out of sync with `directory-abbrev-alist' and
> cause problems (this was a problem with the old code too).

Tramp is prepared :-) See the comment at the beginning of
tramp-cache.el: if you use the connection process as cache key, it isn't
saved persistently. You need to do something like

--8<---------------cut here---------------start------------->8---
(with-tramp-connection-property (tramp-connection-process vec) "abbr-home-directory"
--8<---------------cut here---------------end--------------->8---

> - Jim

Best regards, Michael.

Re: master 3e4d4f472d: Rework `abbreviate-file-name' in Tramp

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

> Tramp is prepared :-) See the comment at the beginning of
> tramp-cache.el: if you use the connection process as cache key, it isn't
> saved persistently. You need to do something like
>
> (with-tramp-connection-property (tramp-connection-process vec) "abbr-home-directory"

Oops, I meant tramp-get-process.

Best regards, Michael.