Subject: bug#71576: closed (Re: bug#71576: 30.0.50; [PATCH] Improve performance of Comint/Eshell password prompt handling)

From: [email-masked] (GNU bug Tracking System)
Date: Fri, 21 Jun 2024 00:44:02 +0000

Your bug report

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

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

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

Subject: Re: bug#71576: 30.0.50; [PATCH] Improve performance of Comint/Eshell password prompt handling
From: Jim Porter <[email-masked]>
To: Stefan Kangas <[email-masked]>, [email-masked]
Date: Thu, 20 Jun 2024 17:42:36 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

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.

Subject: 30.0.50; [PATCH] Improve performance of Comint/Eshell password prompt handling
From: Jim Porter <[email-masked]>
To: [email-masked]
Date: Sat, 15 Jun 2024 12:50:01 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

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.

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