GNU bug report logs - #71576
30.0.50; [PATCH] Improve performance of Comint/Eshell password prompt handling

Previous Next

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

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

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: 30.0.50; [PATCH] Improve performance of Comint/Eshell password prompt
 handling
Date: Sat, 15 Jun 2024 12:50:01 -0700

[Message part 1 (text/plain, inline)]

How long can a password prompt really be? In Comint and Eshell, we check 
for output from subprocesses that looks like a password prompt, so that 
we can hide the password when the user types it in. That's good, but for 
commands that output a lot of text, it can take a while to scan through 
it all.

The attached patch adds a performance optimization for this: since we 
only check for password prompts at the end of a block of output (the 
subprocess is presumably waiting for the user to type in their 
password), we only need to look at the last N characters, where N is 
whatever the maximum password prompt length is. There's obviously no 
*strict* maximum here, but I can't imagine a password prompt being 
longer than 256 characters. Compared to the default 
'read-process-output-max' value of 4096, this means we could skip up to 
93% of the output when looking for the prompt.

In practice, this optimization makes us about 25% faster at outputting 
large amounts of text. For example, in shell-mode, "cat config.log" 
gives me the following results:

 BEFORE |  AFTER
--------+-------
  3.852 |  2.890
  3.728 |  2.771
  3.568 |  2.716

The results in Eshell are pretty similar, too.

I've added NEWS entries for this, although maybe this isn't something 
that really needs to be announced. Still, I figured it was worth 
mentioning in the unlikely case that the new behavior could cause some 
problem with (very!) long password prompts.

I'm also totally fine with letting this patch wait for Emacs 31 if 
there's any concern about the code. It's not a big change, but maybe 
it's worth erring on the side of stability.

[0001-Limit-the-amount-of-text-we-examine-when-looking-for.patch (text/plain, attachment)]

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

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 71576 <at> debbugs.gnu.org
Subject: Re: bug#71576: 30.0.50; [PATCH] Improve performance of
 Comint/Eshell password prompt handling
Date: Sun, 16 Jun 2024 09:45:10 +0200

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

Hi Jim,

> How long can a password prompt really be? In Comint and Eshell, we
> check for output from subprocesses that looks like a password prompt,
> so that we can hide the password when the user types it in. That's
> good, but for commands that output a lot of text, it can take a while
> to scan through it all.
>
> The attached patch adds a performance optimization for this: since we
> only check for password prompts at the end of a block of output (the
> subprocess is presumably waiting for the user to type in their
> password), we only need to look at the last N characters, where N is
> whatever the maximum password prompt length is. There's obviously no
> *strict* maximum here, but I can't imagine a password prompt being
> longer than 256 characters. Compared to the default
> 'read-process-output-max' value of 4096, this means we could skip up
> to 93% of the output when looking for the prompt.

FTR, Tramp scans output for a prompt from the end for years. It's not
only a password prompt, but also a shell prompt it looks for. It
restricts itself to 256 characters.

--8<---------------cut here---------------start------------->8---
(defun tramp-search-regexp (regexp)
  "Search for REGEXP backwards, starting at point-max.
If found, set point to the end of the occurrence found, and return point.
Otherwise, return nil."
  (goto-char (point-max))
  ;; We restrict ourselves to the last 256 characters.  There were
  ;; reports of a shell command "git ls-files -zco --exclude-standard"
  ;; with 85k files involved, which has blocked Tramp forever.
  (search-backward-regexp regexp (max (point-min) (- (point) 256)) 'noerror))
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Jim Porter <jporterbugs <at> gmail.com>, 71576 <at> debbugs.gnu.org
Subject: Re: bug#71576: 30.0.50; [PATCH] Improve performance of Comint/Eshell
 password prompt handling
Date: Sun, 16 Jun 2024 03:53:46 -0700

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

> I've added NEWS entries for this, although maybe this isn't something
> that really needs to be announced. Still, I figured it was worth
> mentioning in the unlikely case that the new behavior could cause some
> problem with (very!) long password prompts.
>
> I'm also totally fine with letting this patch wait for Emacs 31 if
> there's any concern about the code. It's not a big change, but maybe
> it's worth erring on the side of stability.

Thanks.  I'd be okay with putting this patch in Emacs 30, but let's see
what other people think.

> From 197ec6368e6d2678f8f1601dc1a9800855df0943 Mon Sep 17 00:00:00 2001
> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Sat, 15 Jun 2024 11:03:33 -0700
> Subject: [PATCH] Limit the amount of text we examine when looking for password
>  prompts
>
> Both Comint and Eshell do this, and it can significantly slow down
> commands that write a lot of output.
>
> * lisp/comint.el (comint-password-prompt-max-length): New option...
> (comint-watch-for-password-prompt): ... use it.  Additionally, use the
> matched result for the Emacs-based password prompt.
>
> * lisp/eshell/esh-mode.el (eshell-password-prompt-max-length): New
> option...
> (eshell-watch-for-password-prompt): ... use it.
>
> * etc/NEWS: Announce this change.
> ---
>  etc/NEWS                | 21 +++++++++++++++---
>  lisp/comint.el          | 49 +++++++++++++++++++++++++++--------------
>  lisp/eshell/esh-mode.el | 41 +++++++++++++++++++++++-----------
>  3 files changed, 78 insertions(+), 33 deletions(-)
>
> diff --git a/etc/NEWS b/etc/NEWS
> index b2fdbc4a88f..1cf5025910c 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1002,9 +1002,16 @@ more information on this notation.
>  ---
>  *** Performance improvements for interactive output in Eshell.
>  Interactive output in Eshell should now be significantly faster,
> -especially for built-in commands that can print large amounts of output
> -(e.g. "cat").  In addition, these commands can now update the display
> -periodically to show their progress.
> +especially for commands that can print large amounts of output
> +(e.g. "cat").  For external commands, Eshell saves time by only looking
> +for password prompts in the last 256 characters of each block of output.
> +To change the amount of text to examine, customize
> +'eshell-password-prompt-max-length'.
> +
> +---
> +*** Eshell built-in commands can now display progress.
> +Eshell built-in commands like "cat" and "ls" now update the display
> +periodically while running to show their progress.
>
>  +++
>  *** New special reference type '#<marker POSITION BUFFER>'.
> @@ -1160,6 +1167,14 @@ environment variable 'HISTFILE'.
>
>  In a 'shell' buffer, this user option is connection-local.
>
> +---
> +*** Performance improvements for interactive output.
> +Interactive output in Shell mode now scans more selectively for password
> +prompts by only examining the last 256 characters of each block of
> +output, reducing the time spent when printing large amounts of output.
> +To change the amount of text to examine, customize
> +'comint-password-prompt-max-length'.
> +
>  ** Make mode
>
>  *** The Makefile browser is now obsolete.
> diff --git a/lisp/comint.el b/lisp/comint.el
> index 3804932e01c..b8a12074fb7 100644
> --- a/lisp/comint.el
> +++ b/lisp/comint.el
> @@ -426,6 +426,18 @@ comint-password-prompt-regexp
>    :type 'regexp
>    :group 'comint)
>
> +(defcustom comint-password-prompt-max-length 256
> +  "The maximum amount of text to examine when matching password prompts.
> +When non-nil, only examine the last N characters of a block of output.
> +If nil, examine all the output.
> +
> +This is used by `comint-watch-for-password-prompt' to reduce the amount
> +of time spent searching for password prompts."
> +  :version "30.1"
> +  :type '(choice natnum
> +                 (const :tag "Examine all output" nil))
> +  :group 'comint)

If this is hardcoded in Tramp, are we sure that we need this as an
option?  I'd suggest making it into a defconst or defvar instead.

> +
>  ;; Here are the per-interpreter hooks.
>  (defvar comint-get-old-input (function comint-get-old-input-default)
>    "Function that returns old text in Comint mode.
> @@ -2563,23 +2575,26 @@ comint-watch-for-password-prompt
>  carriage returns (\\r) in STRING.
>
>  This function could be in the list `comint-output-filter-functions'."
> -  (when (let ((case-fold-search t))
> -	  (string-match comint-password-prompt-regexp
> -                        (string-replace "\r" "" string)))
> -    ;; Use `run-at-time' in order not to pause execution of the
> -    ;; process filter with a minibuffer
> -    (run-at-time
> -     0 nil
> -     (lambda (current-buf)
> -       (with-current-buffer current-buf
> -         (let ((comint--prompt-recursion-depth
> -                (1+ comint--prompt-recursion-depth)))
> -           (if (> comint--prompt-recursion-depth 10)
> -               (message "Password prompt recursion too deep")
> -             (when (get-buffer-process (current-buffer))
> -               (comint-send-invisible
> -                (string-trim string "[ \n\r\t\v\f\b\a]+" "\n+")))))))
> -     (current-buffer))))
> +  (let ((string (string-limit string comint-password-prompt-max-length t))
> +        prompt)
> +    (when (let ((case-fold-search t))
> +            (string-match comint-password-prompt-regexp
> +                          (string-replace "\r" "" string)))
> +      (setq prompt (string-trim (match-string 0 string)
> +                                "[ \n\r\t\v\f\b\a]+" "\n+"))
> +      ;; Use `run-at-time' in order not to pause execution of the
> +      ;; process filter with a minibuffer
> +      (run-at-time
> +       0 nil
> +       (lambda (current-buf)
> +         (with-current-buffer current-buf
> +           (let ((comint--prompt-recursion-depth
> +                  (1+ comint--prompt-recursion-depth)))
> +             (if (> comint--prompt-recursion-depth 10)
> +                 (message "Password prompt recursion too deep")
> +               (when (get-buffer-process (current-buffer))
> +                 (comint-send-invisible prompt))))))
> +       (current-buffer)))))
>  
>  ;; Low-level process communication
>
> diff --git a/lisp/eshell/esh-mode.el b/lisp/eshell/esh-mode.el
> index ec1a07b7e2f..5603fef90a4 100644
> --- a/lisp/eshell/esh-mode.el
> +++ b/lisp/eshell/esh-mode.el
> @@ -182,6 +182,18 @@ eshell-password-prompt-regexp
>    :type 'regexp
>    :version "27.1")
>
> +(defcustom eshell-password-prompt-max-length 256
> +  "The maximum amount of text to examine when matching password prompts.
> +When non-nil, only examine the last N characters of a block of output.
> +If nil, examine all the output.
> +
> +This is used by `eshell-watch-for-password-prompt' to reduce the amount
> +of time spent searching for password prompts."
> +  :version "30.1"
> +  :type '(choice natnum
> +                 (const :tag "Examine all output" nil))
> +  :group 'comint)
> +
>  (defcustom eshell-skip-prompt-function nil
>    "A function called from beginning of line to skip the prompt."
>    :type '(choice (const nil) function))
> @@ -949,19 +961,22 @@ eshell-watch-for-password-prompt
>  This function could be in the list `eshell-output-filter-functions'."
>    (when (eshell-head-process)
>      (save-excursion
> -      (let ((case-fold-search t))
> -	(goto-char eshell-last-output-block-begin)
> -	(beginning-of-line)
> -	(if (re-search-forward eshell-password-prompt-regexp
> -			       eshell-last-output-end t)
> -            ;; Use `run-at-time' in order not to pause execution of
> -            ;; the process filter with a minibuffer
> -	    (run-at-time
> -             0 nil
> -             (lambda (current-buf)
> -               (with-current-buffer current-buf
> -                 (eshell-send-invisible)))
> -             (current-buffer)))))))
> +      (goto-char (if eshell-password-prompt-max-length
> +                     (max eshell-last-output-block-begin
> +                          (- eshell-last-output-end
> +                             eshell-password-prompt-max-length))
> +                   eshell-last-output-block-begin))
> +      (when (let ((case-fold-search t))
> +              (re-search-forward eshell-password-prompt-regexp
> +                                 eshell-last-output-end t))

Could this be simplified using re-search-backward with the BOUND
argument instead?

> +        ;; Use `run-at-time' in order not to pause execution of the
> +        ;; process filter with a minibuffer.
> +        (run-at-time
> +         0 nil
> +         (lambda (current-buf)
> +           (with-current-buffer current-buf
> +             (eshell-send-invisible)))
> +         (current-buffer))))))
>
>  (custom-add-option 'eshell-output-filter-functions
>  		   'eshell-watch-for-password-prompt)
> --
> 2.25.1

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

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Jim Porter <jporterbugs <at> gmail.com>, 71576 <at> debbugs.gnu.org
Subject: Re: bug#71576: 30.0.50; [PATCH] Improve performance of
 Comint/Eshell password prompt handling
Date: Sun, 16 Jun 2024 14:29:05 +0200

Stefan Kangas <stefankangas <at> gmail.com> writes:

Hi Stefan,

>> +(defcustom comint-password-prompt-max-length 256
>> +  "The maximum amount of text to examine when matching password prompts.
>> +When non-nil, only examine the last N characters of a block of output.
>> +If nil, examine all the output.
>> +
>> +This is used by `comint-watch-for-password-prompt' to reduce the amount
>> +of time spent searching for password prompts."
>> +  :version "30.1"
>> +  :type '(choice natnum
>> +                 (const :tag "Examine all output" nil))
>> +  :group 'comint)
>
> If this is hardcoded in Tramp, are we sure that we need this as an
> option?  I'd suggest making it into a defconst or defvar instead.

Tramp uses this value just "by experience", and to be on the safe
side. I can imagine, that people know that prompts are much shorter in
their case, say 25 bytes. A defcustom is OK in my eyes.

(apply #'max (mapcar #'length password-word-equivalents)) => 14

Best regards, Michael.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, 71576 <at> debbugs.gnu.org
Subject: Re: bug#71576: 30.0.50; [PATCH] Improve performance of Comint/Eshell
 password prompt handling
Date: Sun, 16 Jun 2024 18:18:01 -0700

On 6/16/2024 3:53 AM, Stefan Kangas wrote:
> Thanks.  I'd be okay with putting this patch in Emacs 30, but let's see
> what other people think.

Sounds good to me.

>> +(defcustom comint-password-prompt-max-length 256
>> +  "The maximum amount of text to examine when matching password prompts.
>> +When non-nil, only examine the last N characters of a block of output.
>> +If nil, examine all the output.
>> +
>> +This is used by `comint-watch-for-password-prompt' to reduce the amount
>> +of time spent searching for password prompts."
>> +  :version "30.1"
>> +  :type '(choice natnum
>> +                 (const :tag "Examine all output" nil))
>> +  :group 'comint)
> 
> If this is hardcoded in Tramp, are we sure that we need this as an
> option?  I'd suggest making it into a defconst or defvar instead.

I don't have a strong opinion here, so I'll wait to see if a majority 
forms around this...

>> +      (goto-char (if eshell-password-prompt-max-length
>> +                     (max eshell-last-output-block-begin
>> +                          (- eshell-last-output-end
>> +                             eshell-password-prompt-max-length))
>> +                   eshell-last-output-block-begin))
>> +      (when (let ((case-fold-search t))
>> +              (re-search-forward eshell-password-prompt-regexp
>> +                                 eshell-last-output-end t))
> 
> Could this be simplified using re-search-backward with the BOUND
> argument instead?

I tried, but since I think most of this logic is necessary, it just 
amounted to swapping the 'if' block with 'eshell-last-output-end'. 
Performance doesn't look any different, and I find the current way a bit 
more readable.

(This could probably be simplified if we want to require that 
'eshell-password-prompt-max-length' be non-nil though.)

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, 71576-done <at> debbugs.gnu.org
Subject: Re: bug#71576: 30.0.50; [PATCH] Improve performance of Comint/Eshell
 password prompt handling
Date: Thu, 20 Jun 2024 17:42:36 -0700

On 6/16/2024 3:53 AM, Stefan Kangas wrote:
> Thanks.  I'd be okay with putting this patch in Emacs 30, but let's see
> what other people think.
[snip]
> If this is hardcoded in Tramp, are we sure that we need this as an
> option?  I'd suggest making it into a defconst or defvar instead.

Since no one else has expressed any concerns, I've merged this as 
1a55e957ae5 to the master branch, replacing the defcustoms with defvars.

Previous Next

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