bug#71576: 30.0.50; [PATCH] Improve performance of Comint/Eshell password prompt handling

bug#71576: 30.0.50; [PATCH] Improve performance of Comint/Eshell password prompt handling

[Jim Porter]

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

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.

[-- Attachment #2: 0001-Limit-the-amount-of-text-we-examine-when-looking-for.patch --]
[-- Type: text/plain, Size: 7723 bytes --]

From 197ec6368e6d2678f8f1601dc1a9800855df0943 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@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)
+
 ;; 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)))))
 \f
 ;; 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))
+        ;; 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

bug#71576: 30.0.50; [PATCH] Improve performance of Comint/Eshell password prompt handling

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jim Porter <jporterbugs@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.

bug#71576: 30.0.50; [PATCH] Improve performance of Comint/Eshell password prompt handling

[Stefan Kangas]

Jim Porter <jporterbugs@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@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)))))
>  \f
>  ;; 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

bug#71576: 30.0.50; [PATCH] Improve performance of Comint/Eshell password prompt handling

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefankangas@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.

bug#71576: 30.0.50; [PATCH] Improve performance of Comint/Eshell password prompt handling

[Jim Porter]

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.)

bug#71576: 30.0.50; [PATCH] Improve performance of Comint/Eshell password prompt handling

[Jim Porter]

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.