bug#51699: 29.0.50; [PATCH] Improve performance of 'file-name-case-insensitive-p' for Tramp files

bug#51699: 29.0.50; [PATCH] Improve performance of 'file-name-case-insensitive-p' for Tramp files

[Jim Porter]

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

This is a spinoff of bug#51622. While looking at the performance of 
'abbreviate-file-name' for Tramp files, I noticed that 
'file-name-case-insensitive-p' was taking up a significant percentage of 
the execution time. I dug into this and found two main hot spots:

1) 'tramp-handle-file-name-case-insensitive-p' calling 'file-remote-p' 
and 'expand-file-name'

Since 'file-remote-p' only needed to check whether a connection was 
already established, it could be replaced with this (thanks to Michael 
Albinus for the pointer):

   (let ((non-essential t)) (tramp-connectable-p v))

'expand-file-name' also had room for a small optimization, since it 
previously called 'tramp-connectable-p' (which dissects the file if it's 
not already) and then 'with-parsed-tramp-file-name' (which dissects it 
again). I reversed the order so now there's one fewer dissection, and 
it's a bit faster.

2) Potential handlers in 'tramp-find-foreign-file-name-handler' each 
dissect the file name

Most Tramp methods have a 'tramp-FOO-file-name-p', and most of *those* 
take a file name string and dissect it. This is a lot of duplicated 
effort, so I modified 'tramp-find-foreign-file-name-handler' to pass the 
dissected file name to any of the functions that support it (this is 
indicated by an 'accepts-vec' property on the function). This probably 
warrants some documentation (at least a NEWS entry), but I wanted to be 
sure the strategy made sense before I wrote any docs.

With these changes combined, I see the following results (testing with 
the sshx method connecting to localhost on a GNU/Linux system):

* 'file-name-case-insensitive-p':
   3.5x faster, now 583μs per call
* 'tramp-handle-file-name-case-insensitive-p':
   4.5x faster, now 281μs per call
* 'tramp-find-foreign-file-name-handler':
   5.2x faster, now 45μs per call

In addition to the patches, I've attached the benchmark script that 
generated these results as well as the raw data.

[-- Attachment #2: 0001-Improve-performance-when-checking-case-sensitivity-o.patch --]
[-- Type: text/plain, Size: 2188 bytes --]

From 3be2a5da11018f03c9c0806038058ee8011faf45 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 8 Nov 2021 15:13:34 -0800
Subject: [PATCH 1/2] Improve performance when checking case-sensitivity of
 Tramp file names

* lisp/net/tramp.el (tramp-handle-file-name-case-insensitive-p): Use
'tramp-connectable-p' to test for connection.
* lisp/net/tramp-sh.el (tramp-sh-handle-expand-file-name): Dissect
file name before testing for connectability to reduce duplicated work.
---
 lisp/net/tramp-sh.el | 8 ++++----
 lisp/net/tramp.el    | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lisp/net/tramp-sh.el b/lisp/net/tramp-sh.el
index 6292190940..56dcd61e5c 100644
--- a/lisp/net/tramp-sh.el
+++ b/lisp/net/tramp-sh.el
@@ -2705,11 +2705,11 @@ tramp-sh-handle-expand-file-name
     ;; Unless NAME is absolute, concat DIR and NAME.
     (unless (file-name-absolute-p name)
       (setq name (tramp-compat-file-name-concat dir name)))
+    ;; Dissect NAME.
+    (with-parsed-tramp-file-name name nil
     ;; If connection is not established yet, run the real handler.
-    (if (not (tramp-connectable-p name))
-	(tramp-run-real-handler #'expand-file-name (list name nil))
-      ;; Dissect NAME.
-      (with-parsed-tramp-file-name name nil
+      (if (not (tramp-connectable-p v))
+	  (tramp-run-real-handler #'expand-file-name (list name nil))
 	(unless (tramp-run-real-handler #'file-name-absolute-p (list localname))
 	  (setq localname (concat "~/" localname)))
 	;; Tilde expansion if necessary.  This needs a shell which
diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index b152584c1f..a8972ce69e 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -3484,7 +3484,7 @@ tramp-handle-file-name-case-insensitive-p
      (tramp-get-method-parameter v 'tramp-case-insensitive)
 
      ;; There isn't.  So we must check, in case there's a connection already.
-     (and (file-remote-p filename nil 'connected)
+     (and (let ((non-essential t)) (tramp-connectable-p v))
           (with-tramp-connection-property v "case-insensitive"
 	    (ignore-errors
 	      (with-tramp-progress-reporter v 5 "Checking case-insensitive"
-- 
2.25.1


[-- Attachment #3: 0002-Improve-performance-of-tramp-find-foreign-file-name-.patch --]
[-- Type: text/plain, Size: 12071 bytes --]

From ee711b3ad4f26adc75b8a312e7a05a33a0a68524 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 8 Nov 2021 18:55:44 -0800
Subject: [PATCH 2/2] Improve performance of
 'tramp-find-foreign-file-name-handler'

Previously, most functions in 'tramp-foreign-file-name-handler-alist'
would call 'tramp-dissect-file-name', resulting in it being called
several times whenever 'tramp-find-foreign-file-name-handler' was
called.  Now, functions can indicate they accept a dissected file name
to avoid this duplicated effort.

lisp/net/tramp.el (tramp-ensure-dissected-file-name): New function.
(tramp-find-foreign-file-name-handler): Pass dissected file name to
functions that accept it.
lisp/net/tramp-adb.el (tramp-adb-file-name-p):
lisp/net/tramp-ftp.el (tramp-ftp-file-name-p):
lisp/net/tramp-gvfs.el (tramp-gvfs-file-name-p):
lisp/net/tramp-rclone.el (tramp-rclone-file-name-p):
lisp/net/tramp-smb.el (tramp-smb-file-name-p):
lisp/net/tramp-sshfs.el (tramp-sshfs-file-name-p):
lisp/net/tramp-sudoedit.el (tramp-sudoedit-file-name-p):
Accept dissected file names and mark this with 'accepts-vec' property.
lisp/net/tramp-sh.el (tramp-sh--file-name-p):
New function, and mark it with 'accepts-vec' property.
---
 lisp/net/tramp-adb.el      | 12 +++++++-----
 lisp/net/tramp-ftp.el      | 12 +++++++-----
 lisp/net/tramp-gvfs.el     | 14 ++++++++------
 lisp/net/tramp-rclone.el   | 12 +++++++-----
 lisp/net/tramp-sh.el       | 13 +++++++++++--
 lisp/net/tramp-smb.el      | 12 +++++++-----
 lisp/net/tramp-sshfs.el    | 12 +++++++-----
 lisp/net/tramp-sudoedit.el | 12 +++++++-----
 lisp/net/tramp.el          | 24 ++++++++++++++++++++----
 9 files changed, 81 insertions(+), 42 deletions(-)

diff --git a/lisp/net/tramp-adb.el b/lisp/net/tramp-adb.el
index 362a258f43..64e2dad966 100644
--- a/lisp/net/tramp-adb.el
+++ b/lisp/net/tramp-adb.el
@@ -191,11 +191,13 @@ tramp-adb-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-adb-file-name-p (filename)
-  "Check if it's a FILENAME for ADB."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-adb-method)))
+(defsubst tramp-adb-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for ADB."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-adb-method)))
+
+;;;###tramp-autoload
+(put #'tramp-adb-file-name-p 'accepts-vec t)
 
 ;;;###tramp-autoload
 (defun tramp-adb-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-ftp.el b/lisp/net/tramp-ftp.el
index 11ccdc8a4c..1bdb823d84 100644
--- a/lisp/net/tramp-ftp.el
+++ b/lisp/net/tramp-ftp.el
@@ -175,11 +175,13 @@ tramp-ftp-file-name-handler
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-ftp-file-name-p (filename)
-  "Check if it's a FILENAME that should be forwarded to Ange-FTP."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-ftp-method)))
+(defsubst tramp-ftp-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME that should be forwarded to Ange-FTP."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-ftp-method)))
+
+;;;###tramp-autoload
+(put #'tramp-ftp-file-name-p 'accepts-vec t)
 
 ;;;###tramp-autoload
 (tramp--with-startup
diff --git a/lisp/net/tramp-gvfs.el b/lisp/net/tramp-gvfs.el
index cab912bd93..ccab1f0927 100644
--- a/lisp/net/tramp-gvfs.el
+++ b/lisp/net/tramp-gvfs.el
@@ -834,12 +834,14 @@ tramp-gvfs-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-gvfs-file-name-p (filename)
-  "Check if it's a FILENAME handled by the GVFS daemon."
-  (and (tramp-tramp-file-p filename)
-       (let ((method
-	      (tramp-file-name-method (tramp-dissect-file-name filename))))
-	 (and (stringp method) (member method tramp-gvfs-methods)))))
+(defsubst tramp-gvfs-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME handled by the GVFS daemon."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (let ((method (tramp-file-name-method vec)))
+      (and (stringp method) (member method tramp-gvfs-methods)))))
+
+;;;###tramp-autoload
+(put #'tramp-gvfs-file-name-p 'accepts-vec t)
 
 ;;;###tramp-autoload
 (defun tramp-gvfs-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-rclone.el b/lisp/net/tramp-rclone.el
index 812e06f3f1..52cef2dcb7 100644
--- a/lisp/net/tramp-rclone.el
+++ b/lisp/net/tramp-rclone.el
@@ -156,11 +156,13 @@ tramp-rclone-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-rclone-file-name-p (filename)
-  "Check if it's a FILENAME for rclone."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-rclone-method)))
+(defsubst tramp-rclone-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for rclone."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-rclone-method)))
+
+;;;###tramp-autoload
+(put #'tramp-rclone-file-name-p 'accepts-vec t)
 
 ;;;###tramp-autoload
 (defun tramp-rclone-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-sh.el b/lisp/net/tramp-sh.el
index 56dcd61e5c..db13d92fb3 100644
--- a/lisp/net/tramp-sh.el
+++ b/lisp/net/tramp-sh.el
@@ -3653,11 +3653,20 @@ tramp-sh-file-name-handler-p
 	    (tramp-make-tramp-file-name vec nil 'nohop))
 	   'tramp-sh-file-name-handler)))
 
-;; This must be the last entry, because `identity' always matches.
+;; It must be a `defsubst' in order to push the whole code into
+;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
+;;;###tramp-autoload
+(defsubst tramp-sh--file-name-p (_vec-or-filename) t)
+
+;;;###tramp-autoload
+(put #'tramp-sh--file-name-p 'accepts-vec t)
+
+;; This must be the last entry, because `tramp-sh--file-name-p' always
+;; matches.
 ;;;###tramp-autoload
 (tramp--with-startup
  (tramp-register-foreign-file-name-handler
-  #'identity #'tramp-sh-file-name-handler 'append))
+  #'tramp-sh--file-name-p #'tramp-sh-file-name-handler 'append))
 
 (defun tramp-vc-file-name-handler (operation &rest args)
   "Invoke special file name handler, which collects files to be handled."
diff --git a/lisp/net/tramp-smb.el b/lisp/net/tramp-smb.el
index 49f049d3f3..14ef449745 100644
--- a/lisp/net/tramp-smb.el
+++ b/lisp/net/tramp-smb.el
@@ -330,11 +330,13 @@ tramp-smb-winexe-shell-command-switch
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-smb-file-name-p (filename)
-  "Check if it's a FILENAME for SMB servers."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-smb-method)))
+(defsubst tramp-smb-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for SMB servers."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-smb-method)))
+
+;;;###tramp-autoload
+(put #'tramp-smb-file-name-p 'accepts-vec t)
 
 ;;;###tramp-autoload
 (defun tramp-smb-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-sshfs.el b/lisp/net/tramp-sshfs.el
index a100786345..8210e8a6ae 100644
--- a/lisp/net/tramp-sshfs.el
+++ b/lisp/net/tramp-sshfs.el
@@ -156,11 +156,13 @@ tramp-sshfs-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-sshfs-file-name-p (filename)
-  "Check if it's a FILENAME for sshfs."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-	        tramp-sshfs-method)))
+(defsubst tramp-sshfs-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for sshfs."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-sshfs-method)))
+
+;;;###tramp-autoload
+(put #'tramp-sshfs-file-name-p 'accepts-vec t)
 
 ;;;###tramp-autoload
 (defun tramp-sshfs-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-sudoedit.el b/lisp/net/tramp-sudoedit.el
index 845f31d09b..6ef2160284 100644
--- a/lisp/net/tramp-sudoedit.el
+++ b/lisp/net/tramp-sudoedit.el
@@ -148,11 +148,13 @@ tramp-sudoedit-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-sudoedit-file-name-p (filename)
-  "Check if it's a FILENAME for SUDOEDIT."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-sudoedit-method)))
+(defsubst tramp-sudoedit-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for SUDOEDIT."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-sudoedit-method)))
+
+;;;###tramp-autoload
+(put #'tramp-sudoedit-file-name-p 'accepts-vec t)
 
 ;;;###tramp-autoload
 (defun tramp-sudoedit-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index a8972ce69e..73b69fa1a4 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -1669,6 +1669,16 @@ tramp-dissect-file-name
 
 (put #'tramp-dissect-file-name 'tramp-suppress-trace t)
 
+(defun tramp-ensure-dissected-file-name (vec-or-filename)
+  "Return a `tramp-file-name' structure for VEC-OR-FILENAME.
+
+VEC-OR-FILENAME may be either a string or a `tramp-file-name'.
+If it's not a Tramp filename, return nil."
+  (cond
+   ((tramp-file-name-p vec-or-filename) vec-or-filename)
+   ((tramp-tramp-file-p vec-or-filename)
+    (tramp-dissect-file-name vec-or-filename))))
+
 (defun tramp-dissect-hop-name (name &optional nodefault)
   "Return a `tramp-file-name' structure of `hop' part of NAME.
 See `tramp-dissect-file-name' for details."
@@ -2552,13 +2562,15 @@ tramp-find-foreign-file-name-handler
   "Return foreign file name handler if exists."
   (when (tramp-tramp-file-p filename)
     (let ((handler tramp-foreign-file-name-handler-alist)
+          (vec (tramp-dissect-file-name filename))
 	  elt res)
       (while handler
 	(setq elt (car handler)
 	      handler (cdr handler))
-	(when (funcall (car elt) filename)
-	  (setq handler nil
-		res (cdr elt))))
+        (let ((func (car elt)))
+          (when (funcall func (if (get func 'accepts-vec) vec filename))
+	    (setq handler nil
+		  res (cdr elt)))))
       res)))
 
 ;; Main function.
@@ -2756,7 +2768,11 @@ tramp-register-foreign-file-name-handler
     (func handler &optional append)
   "Register (FUNC . HANDLER) in `tramp-foreign-file-name-handler-alist'.
 FUNC is the function, which determines whether HANDLER is to be called.
-Add operations defined in `HANDLER-alist' to `tramp-file-name-handler'."
+Add operations defined in `HANDLER-alist' to `tramp-file-name-handler'.
+
+If FUNC has the property `accepts-vec' set to a non-nil value, it
+will be called with a dissected filename; otherwise, it will be
+called with a string filename."
   (add-to-list
    'tramp-foreign-file-name-handler-alist `(,func . ,handler) append)
   ;; Mark `operations' the handler is responsible for.
-- 
2.25.1


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

(setq filename "/sshx:localhost:~")
(setq tramp-verbose 0)

;; Make sure we have a connection.
(find-file filename)
;; Check case-insensitivity once to ensure the remote check has run.
(file-name-case-insensitive-p filename)

(message "* file-name-case-insensitive-p (local)")
(benchmark 1000 `(file-name-case-insensitive-p "~"))
(message "* file-name-case-insensitive-p (remote)")
(benchmark 1000 `(file-name-case-insensitive-p ,filename))
(message "* tramp-handle-file-name-case-insensitive-p")
(benchmark 1000 `(tramp-handle-file-name-case-insensitive-p ,filename))
(message "* tramp-find-foreign-file-name-handler")
(benchmark 1000 `(tramp-find-foreign-file-name-handler ,filename))

[-- Attachment #5: benchmark-results.txt --]
[-- Type: text/plain, Size: 1146 bytes --]

Emacs 29 master
---------------

* file-name-case-insensitive-p (local)
  Elapsed time: 0.039413s (0.013177s in 1 GCs)
* file-name-case-insensitive-p (remote)
  Elapsed time: 2.016539s (0.491135s in 37 GCs)
* tramp-handle-file-name-case-insensitive-p
  Elapsed time: 1.262594s (0.312782s in 24 GCs)
* tramp-find-foreign-file-name-handler
  Elapsed time: 0.236242s (0.051822s in 4 GCs)


With patch 1
------------

* file-name-case-insensitive-p (local)
  Elapsed time: 0.039818s (0.013340s in 1 GCs)
* file-name-case-insensitive-p (remote)
  Elapsed time: 1.138076s (0.285803s in 22 GCs)
* tramp-handle-file-name-case-insensitive-p
  Elapsed time: 0.464441s (0.129247s in 10 GCs)
* tramp-find-foreign-file-name-handler
  Elapsed time: 0.233279s (0.051895s in 4 GCs)


With patch 2
------------

* file-name-case-insensitive-p (local)
  Elapsed time: 0.039188s (0.013016s in 1 GCs)
* file-name-case-insensitive-p (remote)
  Elapsed time: 0.583101s (0.189924s in 14 GCs)
* tramp-handle-file-name-case-insensitive-p
  Elapsed time: 0.281154s (0.094141s in 7 GCs)
* tramp-find-foreign-file-name-handler
  Elapsed time: 0.045324s (0.012771s in 1 GCs)

bug#51699: 29.0.50; [PATCH] Improve performance of 'file-name-case-insensitive-p' for Tramp files

[Michael Albinus]

Jim Porter <jporterbugs@gmail.com> writes:

Hi Jim,

> 1) 'tramp-handle-file-name-case-insensitive-p' calling 'file-remote-p'
> and 'expand-file-name'
>
> Since 'file-remote-p' only needed to check whether a connection was
> already established, it could be replaced with this (thanks to Michael
> Albinus for the pointer):
>
>   (let ((non-essential t)) (tramp-connectable-p v))
>
> 'expand-file-name' also had room for a small optimization, since it
> previously called 'tramp-connectable-p' (which dissects the file if
> it's not already) and then 'with-parsed-tramp-file-name' (which
> dissects it again). I reversed the order so now there's one fewer
> dissection, and it's a bit faster.

This is obviously fine, so I've pushed this to master. Thanks for the
improvement!

> 2) Potential handlers in 'tramp-find-foreign-file-name-handler' each
> dissect the file name
>
> Most Tramp methods have a 'tramp-FOO-file-name-p', and most of *those*
> take a file name string and dissect it. This is a lot of duplicated
> effort, so I modified 'tramp-find-foreign-file-name-handler' to pass
> the dissected file name to any of the functions that support it (this
> is indicated by an 'accepts-vec' property on the function). This
> probably warrants some documentation (at least a NEWS entry), but I
> wanted to be sure the strategy made sense before I wrote any docs.

Yes, this makes sense, and it works in my environment (more regression
tests running). I don't understand why you need the 'accepts-vec'
property -- is there any operation left, which is passed to
`tramp-find-foreign-file-name-handler' and which doesn't accept a VEC,
after applying your patch? And if yes, couldn't we apply usual error
handling?

We shall not add this additonal complexity to Tramp.

Best regards, Michael.

bug#51699: 29.0.50; [PATCH] Improve performance of 'file-name-case-insensitive-p' for Tramp files

[Jim Porter]

On 11/9/2021 11:34 AM, Michael Albinus wrote:
> This is obviously fine, so I've pushed this to master. Thanks for the
> improvement!

Thanks for merging!

>> Most Tramp methods have a 'tramp-FOO-file-name-p', and most of *those*
>> take a file name string and dissect it. This is a lot of duplicated
>> effort, so I modified 'tramp-find-foreign-file-name-handler' to pass
>> the dissected file name to any of the functions that support it (this
>> is indicated by an 'accepts-vec' property on the function). This
>> probably warrants some documentation (at least a NEWS entry), but I
>> wanted to be sure the strategy made sense before I wrote any docs.
> 
> Yes, this makes sense, and it works in my environment (more regression
> tests running). I don't understand why you need the 'accepts-vec'
> property -- is there any operation left, which is passed to
> `tramp-find-foreign-file-name-handler' and which doesn't accept a VEC,
> after applying your patch? And if yes, couldn't we apply usual error
> handling?

There's `tramp-archive-file-name-p' and `tramp-crypt-file-name-p', which 
both want a string filename. I looked over the implementation for those 
and couldn't figure out an easy way to convert them to take a VEC. Maybe 
it's possible though. I also looked into passing both the string form 
and the vec form as separate arguments, but that turned out to be even 
more complex than this implementation.

In addition, I did it this way to prevent any breakage for third-party 
code that calls `tramp-register-foreign-file-name-handler'. If we change 
`tramp-find-foreign-file-name-handler' to pass a VEC all the time, then 
any code out there that expects a string will break. This is probably 
rare, but I've seen a few examples of people doing stuff like this over 
the years, e.g. 
<https://github.com/thinkiny/emacs.d/blob/master/lisp/tramp-jumper.el>.

I'm not quite sure what you mean by the "usual error handling" though. 
If there's a simpler way to do this, I'm happy to change the 
implementation. So long as we can minimize the number of times 
`tramp-dissect-file-name' is called, it should be possible to get 
similar performance improvements to the current version of my patch.

bug#51699: 29.0.50; [PATCH] Improve performance of 'file-name-case-insensitive-p' for Tramp files

[Michael Albinus]

Jim Porter <jporterbugs@gmail.com> writes:

Hi Jim,

>>> Most Tramp methods have a 'tramp-FOO-file-name-p', and most of *those*
>>> take a file name string and dissect it. This is a lot of duplicated
>>> effort, so I modified 'tramp-find-foreign-file-name-handler' to pass
>>> the dissected file name to any of the functions that support it (this
>>> is indicated by an 'accepts-vec' property on the function). This
>>> probably warrants some documentation (at least a NEWS entry), but I
>>> wanted to be sure the strategy made sense before I wrote any docs.
>> Yes, this makes sense, and it works in my environment (more
>> regression
>> tests running). I don't understand why you need the 'accepts-vec'
>> property -- is there any operation left, which is passed to
>> `tramp-find-foreign-file-name-handler' and which doesn't accept a VEC,
>> after applying your patch? And if yes, couldn't we apply usual error
>> handling?
>
> There's `tramp-archive-file-name-p' and `tramp-crypt-file-name-p',
> which both want a string filename. I looked over the implementation
> for those and couldn't figure out an easy way to convert them to take
> a VEC. Maybe it's possible though. I also looked into passing both the
> string form and the vec form as separate arguments, but that turned
> out to be even more complex than this implementation.

`tramp-find-foreign-file-name-handler' loops through
`tramp-register-foreign-file-name-handler'. Only operations registered
there need to support a VEC-OR-FILENAME argument.
`tramp-archive-file-name-p' and `tramp-crypt-file-name-p' are not
registered, so no change is needed.

> In addition, I did it this way to prevent any breakage for third-party
> code that calls `tramp-register-foreign-file-name-handler'. If we
> change `tramp-find-foreign-file-name-handler' to pass a VEC all the
> time, then any code out there that expects a string will break. This
> is probably rare, but I've seen a few examples of people doing stuff
> like this over the years,
> e.g. <https://github.com/thinkiny/emacs.d/blob/master/lisp/tramp-jumper.el>.

Yes, and there's also <https://github.com/emacsattic/magit-tramp/blob/master/magit-tramp.el>.
These packages use a lot of internal Tramp functions which are not
documented publicly. So they have always the risk that something fails.

We could (and should) inform the authors of both packages, that the
signature for the `tramp-FOO-file-name-p' functions will change with
Tramp 2.6. As I can see, there's no problem to adapt
`magit-tramp-file-name-p' and `tramp-jumper-file-name-p'. And yes, an
entry in etc/NEWS should be added as well.

> I'm not quite sure what you mean by the "usual error handling"
> though. If there's a simpler way to do this, I'm happy to change the
> implementation. So long as we can minimize the number of times
> `tramp-dissect-file-name' is called, it should be possible to get
> similar performance improvements to the current version of my patch.

The most simple approach in `tramp-find-foreign-file-name-handler' is

--8<---------------cut here---------------start------------->8---
        (when (ignore-errors (funcall (car elt) vec))
--8<---------------cut here---------------end--------------->8---

A  little bit more friendly for debugging:

--8<---------------cut here---------------start------------->8---
	;; The signature of `tramp-FOO-file-name-p' has changed, it
	;; expects a VEC here.
        (when (with-demoted-errors "Error: %S" (funcall (car elt) vec))
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

bug#51699: 29.0.50; [PATCH] Improve performance of 'file-name-case-insensitive-p' for Tramp files

[Jim Porter]

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

On 11/10/2021 4:17 AM, Michael Albinus wrote:
> `tramp-find-foreign-file-name-handler' loops through
> `tramp-register-foreign-file-name-handler'. Only operations registered
> there need to support a VEC-OR-FILENAME argument.
> `tramp-archive-file-name-p' and `tramp-crypt-file-name-p' are not
> registered, so no change is needed.

Ah ha. I didn't realize they weren't included (I thought I just had to 
configure something to add them in 
`tramp-foreign-file-name-handler-alist'). In that case, it definitely 
makes sense eliminate the `accepts-vec' bits from my patch.

> We could (and should) inform the authors of both packages, that the
> signature for the `tramp-FOO-file-name-p' functions will change with
> Tramp 2.6. As I can see, there's no problem to adapt
> `magit-tramp-file-name-p' and `tramp-jumper-file-name-p'. And yes, an
> entry in etc/NEWS should be added as well.

Sounds good to me. I've added a NEWS entry that hopefully explains the 
change fairly clearly. Just to be extra-certain, I re-ran my performance 
tests from before and the results are the same.

> A  little bit more friendly for debugging:
> 
> --8<---------------cut here---------------start------------->8---
> 	;; The signature of `tramp-FOO-file-name-p' has changed, it
> 	;; expects a VEC here.
>          (when (with-demoted-errors "Error: %S" (funcall (car elt) vec))
> --8<---------------cut here---------------end--------------->8---

I went with this since it should make it easier for third parties to 
notice the issue and make the appropriate update to their code. Thanks 
for the pointer.

[-- Attachment #2: 0001-Improve-performance-of-tramp-find-foreign-file-name-.patch --]
[-- Type: text/plain, Size: 11585 bytes --]

From 2aec8e21a3e37728a990c4f116f60c8b12bb2110 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 10 Nov 2021 16:41:00 -0800
Subject: [PATCH] Improve performance of 'tramp-find-foreign-file-name-handler'

Previously, each function in 'tramp-foreign-file-name-handler-alist'
would call 'tramp-dissect-file-name', resulting in it being called
several times whenever 'tramp-find-foreign-file-name-handler' was
called.  Now, functions take the dissected file name to avoid this
duplicated effort.

* lisp/net/tramp.el (tramp-ensure-dissected-file-name): New function.
(tramp-find-foreign-file-name-handler): Pass dissected file name to
functions.
(tramp-connectable-p): Use 'tramp-ensure-dissected-file-name'.

* lisp/net/tramp-adb.el (tramp-adb-file-name-p):
* lisp/net/tramp-ftp.el (tramp-ftp-file-name-p):
* lisp/net/tramp-gvfs.el (tramp-gvfs-file-name-p):
* lisp/net/tramp-rclone.el (tramp-rclone-file-name-p):
* lisp/net/tramp-smb.el (tramp-smb-file-name-p):
* lisp/net/tramp-sshfs.el (tramp-sshfs-file-name-p):
* lisp/net/tramp-sudoedit.el (tramp-sudoedit-file-name-p):
Accept dissected file names.

* etc/NEWS: Announce this change.
---
 etc/NEWS                   |  6 ++++++
 lisp/net/tramp-adb.el      |  9 ++++-----
 lisp/net/tramp-ftp.el      |  9 ++++-----
 lisp/net/tramp-gvfs.el     | 11 +++++------
 lisp/net/tramp-rclone.el   |  9 ++++-----
 lisp/net/tramp-smb.el      |  9 ++++-----
 lisp/net/tramp-sshfs.el    |  9 ++++-----
 lisp/net/tramp-sudoedit.el |  9 ++++-----
 lisp/net/tramp.el          | 26 ++++++++++++++++++--------
 9 files changed, 53 insertions(+), 44 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 807f31fa33..9f3a773a5b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -571,6 +571,12 @@ get proper locale-dependent downcasing, the string has to be converted
 to multibyte first.  (This goes for the other case-changing functions,
 too.)
 
+---
+** Functions in 'tramp-foreign-file-name-handler-alist' have changed.
+Functions to determine which Tramp file name handler to use are now
+passed a file name in dissected form (via 'tramp-dissect-file-name')
+instead of in string form.
+
 ---
 ** 'def' indentation changes.
 In 'emacs-lisp-mode', forms with a symbol with a name that start with
diff --git a/lisp/net/tramp-adb.el b/lisp/net/tramp-adb.el
index 362a258f43..e7fe07e417 100644
--- a/lisp/net/tramp-adb.el
+++ b/lisp/net/tramp-adb.el
@@ -191,11 +191,10 @@ tramp-adb-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-adb-file-name-p (filename)
-  "Check if it's a FILENAME for ADB."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-adb-method)))
+(defsubst tramp-adb-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for ADB."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-adb-method)))
 
 ;;;###tramp-autoload
 (defun tramp-adb-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-ftp.el b/lisp/net/tramp-ftp.el
index 11ccdc8a4c..f78c08ec41 100644
--- a/lisp/net/tramp-ftp.el
+++ b/lisp/net/tramp-ftp.el
@@ -175,11 +175,10 @@ tramp-ftp-file-name-handler
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-ftp-file-name-p (filename)
-  "Check if it's a FILENAME that should be forwarded to Ange-FTP."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-ftp-method)))
+(defsubst tramp-ftp-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME that should be forwarded to Ange-FTP."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-ftp-method)))
 
 ;;;###tramp-autoload
 (tramp--with-startup
diff --git a/lisp/net/tramp-gvfs.el b/lisp/net/tramp-gvfs.el
index cab912bd93..1f9d9d9415 100644
--- a/lisp/net/tramp-gvfs.el
+++ b/lisp/net/tramp-gvfs.el
@@ -834,12 +834,11 @@ tramp-gvfs-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-gvfs-file-name-p (filename)
-  "Check if it's a FILENAME handled by the GVFS daemon."
-  (and (tramp-tramp-file-p filename)
-       (let ((method
-	      (tramp-file-name-method (tramp-dissect-file-name filename))))
-	 (and (stringp method) (member method tramp-gvfs-methods)))))
+(defsubst tramp-gvfs-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME handled by the GVFS daemon."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (let ((method (tramp-file-name-method vec)))
+      (and (stringp method) (member method tramp-gvfs-methods)))))
 
 ;;;###tramp-autoload
 (defun tramp-gvfs-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-rclone.el b/lisp/net/tramp-rclone.el
index 812e06f3f1..64b0176d08 100644
--- a/lisp/net/tramp-rclone.el
+++ b/lisp/net/tramp-rclone.el
@@ -156,11 +156,10 @@ tramp-rclone-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-rclone-file-name-p (filename)
-  "Check if it's a FILENAME for rclone."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-rclone-method)))
+(defsubst tramp-rclone-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for rclone."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-rclone-method)))
 
 ;;;###tramp-autoload
 (defun tramp-rclone-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-smb.el b/lisp/net/tramp-smb.el
index 49f049d3f3..aeabc69246 100644
--- a/lisp/net/tramp-smb.el
+++ b/lisp/net/tramp-smb.el
@@ -330,11 +330,10 @@ tramp-smb-winexe-shell-command-switch
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-smb-file-name-p (filename)
-  "Check if it's a FILENAME for SMB servers."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-smb-method)))
+(defsubst tramp-smb-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for SMB servers."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-smb-method)))
 
 ;;;###tramp-autoload
 (defun tramp-smb-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-sshfs.el b/lisp/net/tramp-sshfs.el
index a100786345..4bc804571e 100644
--- a/lisp/net/tramp-sshfs.el
+++ b/lisp/net/tramp-sshfs.el
@@ -156,11 +156,10 @@ tramp-sshfs-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-sshfs-file-name-p (filename)
-  "Check if it's a FILENAME for sshfs."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-	        tramp-sshfs-method)))
+(defsubst tramp-sshfs-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for sshfs."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-sshfs-method)))
 
 ;;;###tramp-autoload
 (defun tramp-sshfs-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp-sudoedit.el b/lisp/net/tramp-sudoedit.el
index 845f31d09b..48c81a5988 100644
--- a/lisp/net/tramp-sudoedit.el
+++ b/lisp/net/tramp-sudoedit.el
@@ -148,11 +148,10 @@ tramp-sudoedit-file-name-handler-alist
 ;; It must be a `defsubst' in order to push the whole code into
 ;; tramp-loaddefs.el.  Otherwise, there would be recursive autoloading.
 ;;;###tramp-autoload
-(defsubst tramp-sudoedit-file-name-p (filename)
-  "Check if it's a FILENAME for SUDOEDIT."
-  (and (tramp-tramp-file-p filename)
-       (string= (tramp-file-name-method (tramp-dissect-file-name filename))
-		tramp-sudoedit-method)))
+(defsubst tramp-sudoedit-file-name-p (vec-or-filename)
+  "Check if it's a VEC-OR-FILENAME for SUDOEDIT."
+  (when-let* ((vec (tramp-ensure-dissected-file-name vec-or-filename)))
+    (string= (tramp-file-name-method vec) tramp-sudoedit-method)))
 
 ;;;###tramp-autoload
 (defun tramp-sudoedit-file-name-handler (operation &rest args)
diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index a8972ce69e..85effe1a04 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -1669,6 +1669,16 @@ tramp-dissect-file-name
 
 (put #'tramp-dissect-file-name 'tramp-suppress-trace t)
 
+(defun tramp-ensure-dissected-file-name (vec-or-filename)
+  "Return a `tramp-file-name' structure for VEC-OR-FILENAME.
+
+VEC-OR-FILENAME may be either a string or a `tramp-file-name'.
+If it's not a Tramp filename, return nil."
+  (cond
+   ((tramp-file-name-p vec-or-filename) vec-or-filename)
+   ((tramp-tramp-file-p vec-or-filename)
+    (tramp-dissect-file-name vec-or-filename))))
+
 (defun tramp-dissect-hop-name (name &optional nodefault)
   "Return a `tramp-file-name' structure of `hop' part of NAME.
 See `tramp-dissect-file-name' for details."
@@ -2552,11 +2562,14 @@ tramp-find-foreign-file-name-handler
   "Return foreign file name handler if exists."
   (when (tramp-tramp-file-p filename)
     (let ((handler tramp-foreign-file-name-handler-alist)
+          (vec (tramp-dissect-file-name filename))
 	  elt res)
       (while handler
 	(setq elt (car handler)
 	      handler (cdr handler))
-	(when (funcall (car elt) filename)
+        ;; Previously, this function was called with FILENAME, but now
+        ;; it's called with the VEC.
+        (when (with-demoted-errors "Error: %S" (funcall (car elt) vec))
 	  (setq handler nil
 		res (cdr elt))))
       res)))
@@ -2755,8 +2768,9 @@ tramp-register-file-name-handlers
 (defun tramp-register-foreign-file-name-handler
     (func handler &optional append)
   "Register (FUNC . HANDLER) in `tramp-foreign-file-name-handler-alist'.
-FUNC is the function, which determines whether HANDLER is to be called.
-Add operations defined in `HANDLER-alist' to `tramp-file-name-handler'."
+FUNC is the function, which takes a dissected filename and determines
+whether HANDLER is to be called.  Add operations defined in
+`HANDLER-alist' to `tramp-file-name-handler'."
   (add-to-list
    'tramp-foreign-file-name-handler-alist `(,func . ,handler) append)
   ;; Mark `operations' the handler is responsible for.
@@ -2814,11 +2828,7 @@ tramp-connectable-p
 This is true, if either the remote host is already connected, or if we are
 not in completion mode."
   (let ((tramp-verbose 0)
-	(vec
-	 (cond
-	  ((tramp-file-name-p vec-or-filename) vec-or-filename)
-	  ((tramp-tramp-file-p vec-or-filename)
-	   (tramp-dissect-file-name vec-or-filename)))))
+	(vec (tramp-ensure-dissected-file-name vec-or-filename)))
     (or ;; We check this for the process related to
 	;; `tramp-buffer-name'; otherwise `start-file-process'
 	;; wouldn't run ever when `non-essential' is non-nil.
-- 
2.25.1

bug#51699: 29.0.50; [PATCH] Improve performance of 'file-name-case-insensitive-p' for Tramp files

[Michael Albinus]

Version: 29.1

Jim Porter <jporterbugs@gmail.com> writes:

Hi Jim,

> From 2aec8e21a3e37728a990c4f116f60c8b12bb2110 Mon Sep 17 00:00:00 2001
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Wed, 10 Nov 2021 16:41:00 -0800
> Subject: [PATCH] Improve performance of 'tramp-find-foreign-file-name-handler'

Thanks for this final patch. I've applied it to master. It makes
tramp-tests.el faster for most of the test cases, not only for
file-name-case-insensitive-p tests. :-)

Closing this bug.

Best regards, Michael.