Subject: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
From: Jim Porter <[email-masked]>
To: [email-masked]
Date: Mon, 3 Jun 2024 22:36:29 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

In Eshell, if I run "time cat config.log" from my Emacs build directory, 
it reports that it takes about 7.5s. It also doesn't show *any* output 
until it's completely finished. With my attached patches, it now takes 
about 0.6s and also shows the output iteratively, redisplaying 
periodically so users can see that something is happening.

The other command most likely to be impacted by this is the built-in 
version of "ls". When I run "ls -Al /usr/bin" on my system, I go from 
2.1s before my patch to 1.2s after. Not as big an improvement, but still 
noticeable, and it *feels* a lot faster too with the iterative redisplay.

I don't usually add a NEWS entry for perf improvements, but this one 
seemed notable enough that I figured it was worth tooting my own horn. :)

From 0533c21f4b509e61a73eec6b29b93104202c3bf8 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Mon, 3 Jun 2024 22:01:48 -0700
Subject: [PATCH 1/2] Be more efficient when buffering output in Eshell

This makes the built-in 'eshell/cat' 5-10x faster on large files in my
(somewhat limited) tests.  In addition, this change periodically
redisplays when using the Eshell buffered output so that users can see
some progress.

* lisp/eshell/esh-io.el (eshell-print-queue-size): Make obsolete in
favor of...
(eshell-buffered-print-size): ... this.
(eshell-buffered-print-redisplay-throttle): New user option.
(eshell-print-queue): Make local.
(eshell--next-redisplay-time): New variable.
(eshell-print-queue-count): Make obsolete in favor of...
(eshell-print-queue-size): ... this.
(eshell-init-print-buffer): Make obsolete.
(eshell-flush): Simplify.
(eshell-buffered-print): Compare queued output length to
'eshell-buffered-print-size'.
(eshell-with-buffered-print): New macro.

* lisp/eshell/esh-var.el (eshell/env):
* lisp/eshell/em-dirs.el (eshell/cd):
* lisp/eshell/em-hist.el (eshell/history):
* lisp/eshell/em-unix.el (eshell/cat):
* lisp/eshell/em-ls.el (eshell/ls): Use 'eshell-with-buffered-print'.
(flush-func): Remove.
(eshell-ls--insert-directory, eshell-do-ls): Remove 'flush-func'.

* etc/NEWS: Announce these improvements.
---
 etc/NEWS               |  7 ++++
 lisp/eshell/em-dirs.el | 13 +++---
 lisp/eshell/em-hist.el | 13 +++---
 lisp/eshell/em-ls.el   | 14 +++----
 lisp/eshell/em-unix.el | 25 ++++++------
 lisp/eshell/esh-io.el  | 89 +++++++++++++++++++++++++++++++-----------
 lisp/eshell/esh-var.el |  7 ++--
 7 files changed, 105 insertions(+), 63 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 302cd30a135..b2c8e7439e7 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -948,6 +948,13 @@ files and deny read permission for users who are not members of the
 file's group.  See the Info node "(coreutils) File permissions" for
 more information on this notation.
 
+---
+*** Performance improvements for interactive output in Eshell.
+Interactive output in Eshell should now be significnatly 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.
+
 +++
 *** New special reference type '#<marker POSITION BUFFER>'.
 This special reference type returns a marker at 'POSITION' in
diff --git a/lisp/eshell/em-dirs.el b/lisp/eshell/em-dirs.el
index a3d1a349540..e70f2cfe196 100644
--- a/lisp/eshell/em-dirs.el
+++ b/lisp/eshell/em-dirs.el
@@ -400,13 +400,12 @@ eshell/cd
 		(index 0))
 	    (if (= len 0)
 		(error "Directory ring empty"))
-	    (eshell-init-print-buffer)
-	    (while (< index len)
-	      (eshell-buffered-print
-	       (concat (number-to-string index) ": "
-		       (ring-ref eshell-last-dir-ring index) "\n"))
-	      (setq index (1+ index)))
-	    (eshell-flush)
+            (eshell-with-buffered-print
+              (while (< index len)
+                (eshell-buffered-print
+                 (concat (number-to-string index) ": "
+                         (ring-ref eshell-last-dir-ring index) "\n"))
+                (setq index (1+ index))))
 	    (setq handled t)))))
      (path
       (setq path (eshell-expand-multiple-dots path))))
diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index 8865cc745a3..9ffddfb611f 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -333,7 +333,6 @@ eshell-save-some-history
 
 (defun eshell/history (&rest args)
   "List in help buffer the buffer's input history."
-  (eshell-init-print-buffer)
   (eshell-eval-using-options
    "history" args
    '((?r "read" nil read-history
@@ -370,12 +369,12 @@ eshell/history
        (let* ((index (1- (or length (ring-length eshell-history-ring))))
 	      (ref (- (ring-length eshell-history-ring) index)))
 	 ;; We have to build up a list ourselves from the ring vector.
-	 (while (>= index 0)
-	   (eshell-buffered-print
-	    (format "%5d  %s\n" ref (eshell-get-history index)))
-	   (setq index (1- index)
-		 ref (1+ ref)))))))
-   (eshell-flush)
+         (eshell-with-buffered-print
+           (while (>= index 0)
+             (eshell-buffered-print
+              (format "%5d  %s\n" ref (eshell-get-history index)))
+             (setq index (1- index)
+                   ref (1+ ref))))))))
    nil))
 
 (defun eshell-put-history (input &optional ring at-beginning)
diff --git a/lisp/eshell/em-ls.el b/lisp/eshell/em-ls.el
index 82d4b01393f..8bf2e20d320 100644
--- a/lisp/eshell/em-ls.el
+++ b/lisp/eshell/em-ls.el
@@ -229,7 +229,6 @@ block-size
 (defvar dereference-links)
 (defvar dir-literal)
 (defvar error-func)
-(defvar flush-func)
 (defvar human-readable)
 (defvar ignore-pattern)
 (defvar insert-func)
@@ -278,7 +277,6 @@ eshell-ls--insert-directory
           (require 'em-glob)
           (let* ((insert-func 'insert)
                  (error-func 'insert)
-                 (flush-func 'ignore)
                  (eshell-error-if-no-glob t)
                  (target ; Expand the shell wildcards if any.
                   (if (and (atom file)
@@ -324,10 +322,10 @@ eshell-ls--dired
 
 (defsubst eshell/ls (&rest args)
   "An alias version of `eshell-do-ls'."
-  (let ((insert-func 'eshell-buffered-print)
-	(error-func 'eshell-error)
-	(flush-func 'eshell-flush))
-    (apply 'eshell-do-ls args)))
+  (eshell-with-buffered-print
+    (let ((insert-func #'eshell-buffered-print)
+          (error-func #'eshell-error))
+      (apply 'eshell-do-ls args))))
 
 (put 'eshell/ls 'eshell-no-numeric-conversions t)
 (put 'eshell/ls 'eshell-filename-arguments t)
@@ -336,7 +334,6 @@ eshell/ls
 
 (defun eshell-do-ls (&rest args)
   "Implementation of \"ls\" in Lisp, passing ARGS."
-  (funcall flush-func -1)
   ;; Process the command arguments, and begin listing files.
   (eshell-eval-using-options
    "ls" (if eshell-ls-initial-args
@@ -422,8 +419,7 @@ eshell-do-ls
 		      (eshell-file-attributes
 		       arg (if numeric-uid-gid 'integer 'string))))
 	      args)
-      t (expand-file-name default-directory)))
-   (funcall flush-func)))
+      t (expand-file-name default-directory)))))
 
 (defsubst eshell-ls-printable-size (filesize &optional by-blocksize)
   "Return a printable FILESIZE."
diff --git a/lisp/eshell/em-unix.el b/lisp/eshell/em-unix.el
index 4137c05fa41..e6bd0381a14 100644
--- a/lisp/eshell/em-unix.el
+++ b/lisp/eshell/em-unix.el
@@ -659,7 +659,6 @@ eshell/cat
 	  (if eshell-in-pipeline-p
 	      (error "Eshell's `cat' does not work in pipelines")
 	    (error "Eshell's `cat' cannot display one of the files given"))))
-    (eshell-init-print-buffer)
     (eshell-eval-using-options
      "cat" args
      '((?h "help" nil nil "show this usage screen")
@@ -672,18 +671,18 @@ eshell/cat
 	   (throw 'eshell-external
 		  (eshell-external-command "cat" args))))
      (let ((curbuf (current-buffer)))
-       (dolist (file args)
-	 (with-temp-buffer
-	   (insert-file-contents file)
-	   (goto-char (point-min))
-	   (while (not (eobp))
-	     (let ((str (buffer-substring
-			 (point) (min (1+ (line-end-position))
-				      (point-max)))))
-	       (with-current-buffer curbuf
-		 (eshell-buffered-print str)))
-	     (forward-line)))))
-     (eshell-flush))))
+       (eshell-with-buffered-print
+         (dolist (file args)
+	   (with-temp-buffer
+	     (insert-file-contents file)
+	     (goto-char (point-min))
+             (while (not (eobp))
+               (let* ((pos (min (+ (point) eshell-buffered-print-size)
+                                (point-max)))
+                      (str (buffer-substring (point) pos)))
+                 (with-current-buffer curbuf
+                   (eshell-buffered-print str))
+                 (goto-char pos))))))))))
 
 (put 'eshell/cat 'eshell-no-numeric-conversions t)
 (put 'eshell/cat 'eshell-filename-arguments t)
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index c7017ee1d70..7587b7ddac9 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -112,10 +112,28 @@ eshell-error-handle
 
 (defcustom eshell-print-queue-size 5
   "The size of the print queue, for doing buffered printing.
+This variable is obsolete.  You should use `eshell-buffered-print-size'
+instead."
+  :type 'integer
+  :group 'eshell-io)
+(make-obsolete-variable 'eshell-print-queue-size
+                        'eshell-buffered-print-size "30.1")
+
+(defcustom eshell-buffered-print-size 2048
+  "The size of the print queue in characters, for doing buffered printing.
 This is basically a speed enhancement, to avoid blocking the Lisp code
 from executing while Emacs is redisplaying."
   :type 'integer
-  :group 'eshell-io)
+  :group 'eshell-io
+  :version "30.1")
+
+(defcustom eshell-buffered-print-redisplay-throttle 0.025
+  "The minimum time in seconds between redisplays when using buffered printing.
+If nil, don't redisplay while printing."
+  :type '(choice number
+                 (const :tag "Don't redisplay" nil))
+  :group 'eshell-io
+  :version "30.1")
 
 (defcustom eshell-virtual-targets
   '(;; The literal string "/dev/null" is intentional here.  It just
@@ -460,40 +478,65 @@ eshell-interactive-output-p
              (equal (caar (aref handles eshell-error-handle)) '(t)))
       (equal (caar (aref handles index)) '(t)))))
 
-(defvar eshell-print-queue nil)
+(defvar-local eshell-print-queue nil)
+(defvar-local eshell-print-queue-size nil)
+(defvar eshell--next-redisplay-time nil)
+
 (defvar eshell-print-queue-count -1)
+(make-obsolete-variable 'eshell-print-queue-count
+                        'eshell-print-queue-size "30.1")
 
 (defsubst eshell-print (object)
   "Output OBJECT to the standard output handle."
   (eshell-output-object object eshell-output-handle))
 
-(defun eshell-flush (&optional reset-p)
-  "Flush out any lines that have been queued for printing.
-Must be called before printing begins with -1 as its argument, and
-after all printing is over with no argument."
-  (ignore
-   (if reset-p
-       (setq eshell-print-queue nil
-	     eshell-print-queue-count reset-p)
-     (if eshell-print-queue
-	 (eshell-print eshell-print-queue))
-     (eshell-flush 0))))
-
 (defun eshell-init-print-buffer ()
   "Initialize the buffered printing queue."
+  (declare (obsolete #'eshell-with-buffered-print "30.1"))
   (eshell-flush -1))
 
+(defun eshell-flush (&optional clear)
+  "Flush out any lines that have been queued for printing.
+If CLEAR is non-nil, just delete the existing lines instead of printing
+them."
+  (when eshell-print-queue
+    (unless clear
+      (eshell-print (apply #'concat eshell-print-queue))
+      ;; When printing interactively (see `eshell-with-buffered-print'),
+      ;; periodically redisplay so the user can see some progress.
+      (when (and eshell--next-redisplay-time
+                 (time-less-p eshell--next-redisplay-time (current-time)))
+        (redisplay)
+        (setq eshell--next-redisplay-time
+              (time-add eshell--next-redisplay-time
+                        eshell-buffered-print-redisplay-throttle))))
+    (setq eshell-print-queue nil
+          eshell-print-queue-size 0)))
+
 (defun eshell-buffered-print (&rest strings)
   "A buffered print -- *for strings only*."
-  (if (< eshell-print-queue-count 0)
-      (progn
-	(eshell-print (apply 'concat strings))
-	(setq eshell-print-queue-count 0))
-    (if (= eshell-print-queue-count eshell-print-queue-size)
-	(eshell-flush))
-    (setq eshell-print-queue
-	  (concat eshell-print-queue (apply 'concat strings))
-	  eshell-print-queue-count (1+ eshell-print-queue-count))))
+  (setq eshell-print-queue
+        (nconc eshell-print-queue strings)
+        eshell-print-queue-size
+        (+ eshell-print-queue-size (apply #'+ (mapcar #'length strings))))
+  (when (> eshell-print-queue-size eshell-buffered-print-size)
+    (eshell-flush)))
+
+(defmacro eshell-with-buffered-print (&rest body)
+  "Initialize buffered printing for Eshell, and then evaluate BODY.
+When printing interactively, this will call `redisplay' every
+`eshell-buffered-print-redisplay-throttle' seconds so that the user can
+see the progress."
+  (declare (indent 0))
+  `(unwind-protect
+       (let ((eshell--next-redisplay-time
+              (when (and eshell-buffered-print-redisplay-throttle
+                         (eshell-interactive-output-p))
+                (time-add (current-time)
+                          eshell-buffered-print-redisplay-throttle))))
+          (eshell-flush t)
+          ,@body)
+     (eshell-flush)))
 
 (defsubst eshell-error (object)
   "Output OBJECT to the standard error handle."
diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index 02b5c785625..f0270aca92c 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -437,10 +437,9 @@ eshell/env
    (if args
        (or (eshell-parse-local-variables args)
            (eshell-named-command (car args) (cdr args)))
-     (eshell-init-print-buffer)
-     (dolist (setting (sort (eshell-environment-variables) 'string-lessp))
-       (eshell-buffered-print setting "\n"))
-     (eshell-flush))))
+     (eshell-with-buffered-print
+       (dolist (setting (sort (eshell-environment-variables) 'string-lessp))
+         (eshell-buffered-print setting "\n"))))))
 
 (defun eshell-insert-envvar (envvar-name)
   "Insert ENVVAR-NAME into the current buffer at point."
-- 
2.25.1

From 7af9d8d00cf4c1c9899ddbe7d1e10129d261c09e Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Mon, 3 Jun 2024 22:06:49 -0700
Subject: [PATCH 2/2] Improve implementations of some Eshell output filter
 functions

* lisp/eshell/esh-mode.el (eshell-postoutput-scroll-to-bottom): Use
'get-buffer-window-list' for simplicity.
(eshell-handle-control-codes): Use 're-search-forward'; this way is much
faster.

* test/lisp/eshell/esh-mode-tests.el: New file.
---
 lisp/eshell/esh-mode.el            | 68 +++++++++++++-----------------
 test/lisp/eshell/esh-mode-tests.el | 62 +++++++++++++++++++++++++++
 2 files changed, 92 insertions(+), 38 deletions(-)
 create mode 100644 test/lisp/eshell/esh-mode-tests.el

diff --git a/lisp/eshell/esh-mode.el b/lisp/eshell/esh-mode.el
index e6f3cb5f6ad..ec1a07b7e2f 100644
--- a/lisp/eshell/esh-mode.el
+++ b/lisp/eshell/esh-mode.el
@@ -765,30 +765,25 @@ eshell-postoutput-scroll-to-bottom
 	 (current (current-buffer))
 	 (scroll eshell-scroll-to-bottom-on-output))
     (unwind-protect
-	(walk-windows
-         (lambda (window)
-           (if (eq (window-buffer window) current)
-               (progn
-                 (select-window window)
-                 (if (and (< (point) eshell-last-output-end)
-                          (or (eq scroll t) (eq scroll 'all)
-                              ;; Maybe user wants point to jump to end.
-                              (and (eq scroll 'this)
-                                   (eq selected window))
-                              (and (eq scroll 'others)
-                                   (not (eq selected window)))
-                              ;; If point was at the end, keep it at end.
-                              (>= (point) eshell-last-output-start)))
-                     (goto-char eshell-last-output-end))
-                 ;; Optionally scroll so that the text
-                 ;; ends at the bottom of the window.
-                 (if (and eshell-scroll-show-maximum-output
-                          (>= (point) eshell-last-output-end))
-                     (save-excursion
-                       (goto-char (point-max))
-                       (recenter -1)))
-                 (select-window selected))))
-	 nil t)
+        (dolist (window (get-buffer-window-list current nil t))
+          (with-selected-window window
+            (when (and (< (point) eshell-last-output-end)
+                       (or (eq scroll t) (eq scroll 'all)
+                           ;; Maybe user wants point to jump to end.
+                           (and (eq scroll 'this)
+                                (eq selected window))
+                           (and (eq scroll 'others)
+                                (not (eq selected window)))
+                           ;; If point was at the end, keep it at end.
+                           (>= (point) eshell-last-output-start)))
+              (goto-char eshell-last-output-end))
+            ;; Optionally scroll so that the text ends at the bottom of
+            ;; the window.
+            (when (and eshell-scroll-show-maximum-output
+                       (>= (point) eshell-last-output-end))
+              (save-excursion
+                (goto-char (point-max))
+                (recenter -1)))))
       (set-buffer current))))
 
 (defun eshell-beginning-of-input ()
@@ -977,27 +972,24 @@ eshell-handle-control-codes
     (goto-char eshell-last-output-block-begin)
     (unless (eolp)
       (beginning-of-line))
-    (while (< (point) eshell-last-output-end)
-      (let ((char (char-after)))
+    (while (re-search-forward (rx (any ?\r ?\a ?\C-h))
+                              eshell-last-output-end t)
+      (let ((char (char-before)))
         (cond
          ((eq char ?\r)
-          (if (< (1+ (point)) eshell-last-output-end)
-              (if (memq (char-after (1+ (point)))
-                        '(?\n ?\r))
-                  (delete-char 1)
-                (let ((end (1+ (point))))
+          (if (< (point) eshell-last-output-end)
+              (if (memq (char-after (point)) '(?\n ?\r))
+                  (delete-char -1)
+                (let ((end (point)))
                   (beginning-of-line)
                   (delete-region (point) end)))
-            (add-text-properties (point) (1+ (point))
-                                 '(invisible t))
-            (forward-char)))
+            (add-text-properties (1- (point)) (point)
+                                 '(invisible t))))
          ((eq char ?\a)
-          (delete-char 1)
+          (delete-char -1)
           (beep))
          ((eq char ?\C-h)
-          (delete-region (1- (point)) (1+ (point))))
-         (t
-          (forward-char)))))))
+          (delete-region (- (point) 2) (point))))))))
 
 (custom-add-option 'eshell-output-filter-functions
 		   'eshell-handle-control-codes)
diff --git a/test/lisp/eshell/esh-mode-tests.el b/test/lisp/eshell/esh-mode-tests.el
new file mode 100644
index 00000000000..306e11ce445
--- /dev/null
+++ b/test/lisp/eshell/esh-mode-tests.el
@@ -0,0 +1,62 @@
+;;; esh-mode-tests.el --- esh-mode test suite  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2022-2024 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Tests for Eshell's command invocation.
+
+;;; Code:
+
+(require 'ert)
+(require 'esh-mode)
+(require 'eshell)
+
+(require 'eshell-tests-helpers
+         (expand-file-name "eshell-tests-helpers"
+                           (file-name-directory (or load-file-name
+                                                    default-directory))))
+
+;;; Tests:
+
+(ert-deftest esh-mode-test/handle-control-codes/carriage-return ()
+  "Test that Eshell handles carriage returns properly."
+  (with-temp-eshell
+    (eshell-match-command-output "(format \"hello\r\ngoodbye\")"
+                                 "\\`hello\ngoodbye\n")
+    (eshell-match-command-output "(format \"hello\rgoodbye\")"
+                                 "\\`goodbye\n")
+    (eshell-match-command-output "(format \"hello\r\")"
+                                 "\\`hello")))
+
+(ert-deftest esh-mode-test/handle-control-codes/bell ()
+  "Test that Eshell handles bells properly."
+  (cl-letf* ((beep-called nil)
+             ((symbol-function 'beep) (lambda () (setq beep-called t))))
+    (with-temp-eshell
+      (eshell-match-command-output "(format \"hello\athere\")"
+                                   "\\`hellothere\n")
+      (should beep-called))))
+
+(ert-deftest esh-mode-test/handle-control-codes/backspace ()
+  "Test that Eshell handles backspaces properly."
+  (with-temp-eshell
+    (eshell-match-command-output (format "(format \"hello%c%cp\")" ?\C-h ?\C-h)
+                                 "\\`help\n")))
+
+;; esh-mode-tests.el ends here
-- 
2.25.1