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

From: [email-masked] (GNU bug Tracking System)
Date: Thu, 11 Nov 2021 18:52:02 +0000

Your bug report

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

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
51699: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=51699
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#51699: 29.0.50; [PATCH] Improve performance of 'file-name-case-insensitive-p' for Tramp files
From: Michael Albinus <[email-masked]>
To: Jim Porter <[email-masked]>
Cc: [email-masked]
Date: Thu, 11 Nov 2021 19:51:14 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

Version: 29.1

Jim Porter <[email-masked]> writes:

Hi Jim,

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

Subject: 29.0.50; [PATCH] Improve performance of 'file-name-case-insensitive-p' for Tramp files
From: Jim Porter <[email-masked]>
To: [email-masked]
Date: Mon, 8 Nov 2021 19:46:05 -0800
List-Id: <debbugs-submit.debbugs.gnu.org>

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