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

Previous Next

Package: emacs;

Reported by: Jim Porter <jporterbugs <at> gmail.com>

Date: Tue, 9 Nov 2021 03:53:02 UTC

Severity: normal

Tags: patch

Found in version 29.0.50

Fixed in version 29.1

Done: Michael Albinus <michael.albinus <at> gmx.de>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 51699 in the body.
You can then email your comments to 51699 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#51699; Package emacs. (Tue, 09 Nov 2021 03:53:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Jim Porter <jporterbugs <at> gmail.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Tue, 09 Nov 2021 03:53:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; [PATCH] Improve performance of
 'file-name-case-insensitive-p' for Tramp files
Date: Mon, 8 Nov 2021 19:46:05 -0800
[Message part 1 (text/plain, inline)]
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.
[0001-Improve-performance-when-checking-case-sensitivity-o.patch (text/plain, attachment)]
[0002-Improve-performance-of-tramp-find-foreign-file-name-.patch (text/plain, attachment)]
[benchmark.el (text/plain, attachment)]
[benchmark-results.txt (text/plain, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#51699; Package emacs. (Tue, 09 Nov 2021 19:35:01 GMT) Full text and rfc822 format available.

Message #8 received at 51699 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 51699 <at> debbugs.gnu.org
Subject: Re: bug#51699: 29.0.50; [PATCH] Improve performance of
 'file-name-case-insensitive-p' for Tramp files
Date: Tue, 09 Nov 2021 20:34:01 +0100
Jim Porter <jporterbugs <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#51699; Package emacs. (Tue, 09 Nov 2021 21:23:02 GMT) Full text and rfc822 format available.

Message #11 received at 51699 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 51699 <at> debbugs.gnu.org
Subject: Re: bug#51699: 29.0.50; [PATCH] Improve performance of
 'file-name-case-insensitive-p' for Tramp files
Date: Tue, 9 Nov 2021 13:22:11 -0800
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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#51699; Package emacs. (Wed, 10 Nov 2021 12:18:01 GMT) Full text and rfc822 format available.

Message #14 received at 51699 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 51699 <at> debbugs.gnu.org
Subject: Re: bug#51699: 29.0.50; [PATCH] Improve performance of
 'file-name-case-insensitive-p' for Tramp files
Date: Wed, 10 Nov 2021 13:17:08 +0100
Jim Porter <jporterbugs <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#51699; Package emacs. (Thu, 11 Nov 2021 00:49:02 GMT) Full text and rfc822 format available.

Message #17 received at 51699 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 51699 <at> debbugs.gnu.org
Subject: Re: bug#51699: 29.0.50; [PATCH] Improve performance of
 'file-name-case-insensitive-p' for Tramp files
Date: Wed, 10 Nov 2021 16:48:03 -0800
[Message part 1 (text/plain, inline)]
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.
[0001-Improve-performance-of-tramp-find-foreign-file-name-.patch (text/plain, attachment)]

Reply sent to Michael Albinus <michael.albinus <at> gmx.de>:
You have taken responsibility. (Thu, 11 Nov 2021 18:52:02 GMT) Full text and rfc822 format available.

Notification sent to Jim Porter <jporterbugs <at> gmail.com>:
bug acknowledged by developer. (Thu, 11 Nov 2021 18:52:02 GMT) Full text and rfc822 format available.

Message #22 received at 51699-done <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 51699-done <at> debbugs.gnu.org
Subject: Re: bug#51699: 29.0.50; [PATCH] Improve performance of
 'file-name-case-insensitive-p' for Tramp files
Date: Thu, 11 Nov 2021 19:51:14 +0100
Version: 29.1

Jim Porter <jporterbugs <at> gmail.com> writes:

Hi Jim,

> From 2aec8e21a3e37728a990c4f116f60c8b12bb2110 Mon Sep 17 00:00:00 2001
> From: Jim Porter <jporterbugs <at> 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.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Fri, 10 Dec 2021 12:24:14 GMT) Full text and rfc822 format available.

This bug report was last modified 2 years and 99 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.