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

From: Jim Porter <[email-masked]>
Date: Mon, 8 Nov 2021 19:46:05 -0800

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.
From 3be2a5da11018f03c9c0806038058ee8011faf45 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
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

From ee711b3ad4f26adc75b8a312e7a05a33a0a68524 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
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