* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
@ 2024-06-04 5:36 Jim Porter
2024-06-04 21:52 ` Stefan Kangas
0 siblings, 1 reply; 22+ messages in thread
From: Jim Porter @ 2024-06-04 5:36 UTC (permalink / raw)
To: 71355
[-- Attachment #1: Type: text/plain, Size: 775 bytes --]
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. :)
[-- Attachment #2: 0001-Be-more-efficient-when-buffering-output-in-Eshell.patch --]
[-- Type: text/plain, Size: 13443 bytes --]
From 0533c21f4b509e61a73eec6b29b93104202c3bf8 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
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
[-- Attachment #3: 0002-Improve-implementations-of-some-Eshell-output-filter.patch --]
[-- Type: text/plain, Size: 7465 bytes --]
From 7af9d8d00cf4c1c9899ddbe7d1e10129d261c09e Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-04 5:36 bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell Jim Porter
@ 2024-06-04 21:52 ` Stefan Kangas
2024-06-05 1:55 ` Jim Porter
0 siblings, 1 reply; 22+ messages in thread
From: Stefan Kangas @ 2024-06-04 21:52 UTC (permalink / raw)
To: Jim Porter, 71355
Jim Porter <jporterbugs@gmail.com> writes:
> 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. :)
Nice, thanks for working on this. Your patch makes sense to me at first
glance, but I didn't test it. The performance improvement definitely
seems highly worthwhile based on your measurements.
Bonus points for adding tests, as always.
> +(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."
How did you decide on this value?
Could the docstring be expanded to explain what a user can expect to
happen if they increase or decrease this value?
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-04 21:52 ` Stefan Kangas
@ 2024-06-05 1:55 ` Jim Porter
2024-06-05 3:50 ` Jim Porter
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Jim Porter @ 2024-06-05 1:55 UTC (permalink / raw)
To: Stefan Kangas, 71355
On 6/4/2024 2:52 PM, Stefan Kangas wrote:
> Nice, thanks for working on this. Your patch makes sense to me at first
> glance, but I didn't test it. The performance improvement definitely
> seems highly worthwhile based on your measurements.
If anyone has time to test, I'd be interested to see if the results are
similarly good on other platforms (I'm testing on x86_64 GNU/Linux), but
I imagine they're comparable. The main areas of improvement are:
1. Batching output into larger chunks reduces the constant-time
overheads associated with each write to the screen.
2. Scanning for control characters in output is much faster thanks to
're-search-forward'.
3. The above improvements made it possible to add periodic redisplays,
which eat into the perf improvements. I mitigated this by throttling the
redisplay to 40Hz (the slowest value I found where the throttling isn't
obvious).
> Bonus points for adding tests, as always.
I wrote some tests for the control character fix, since my changes to
that function were nontrivial. I'll see if I can think of a few others,
though I'm partly relying on the now-fairly-extensive Eshell test suite
to catch any mistakes.
>> +(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."
>
> How did you decide on this value?
Basically, I tried a few different command invocations that normally
take a while (the main ones being "cat config.log" and "ls -Al
/usr/bin") and scaled up the value by factors of two until I stopped
seeing any major perf improvements. (Larger values are faster still, but
not by a whole lot.)
> Could the docstring be expanded to explain what a user can expect to
> happen if they increase or decrease this value?
Sure, that makes sense. Essentially, smaller values will be slower, but
may update faster (subject to the redisplay throttling), whereas larger
values are the opposite.
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-05 1:55 ` Jim Porter
@ 2024-06-05 3:50 ` Jim Porter
2024-06-05 12:06 ` Eli Zaretskii
2024-06-06 9:20 ` Stefan Kangas
2024-06-06 23:14 ` Stefan Kangas
2 siblings, 1 reply; 22+ messages in thread
From: Jim Porter @ 2024-06-05 3:50 UTC (permalink / raw)
To: Stefan Kangas, 71355
[-- Attachment #1: Type: text/plain, Size: 509 bytes --]
On 6/4/2024 6:55 PM, Jim Porter wrote:
> On 6/4/2024 2:52 PM, Stefan Kangas wrote:
>> Could the docstring be expanded to explain what a user can expect to
>> happen if they increase or decrease this value?
>
> Sure, that makes sense. Essentially, smaller values will be slower, but
> may update faster (subject to the redisplay throttling), whereas larger
> values are the opposite.
I've expanded this docstring and a few others, plus added a simple test
to make sure Eshell's built-in "cat" still works.
[-- Attachment #2: 0001-Be-more-efficient-when-buffering-output-in-Eshell.patch --]
[-- Type: text/plain, Size: 17308 bytes --]
From 1898d45d9fd02d3d52208db4c84191e5cb8284f6 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
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): Add new REDISPLAY-NOW argument in favor of CLEAR (which
only 'eshell-init-print-buffer' should have used).
(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'.
* test/lisp/eshell/em-unix-tests.el (em-unix-test/compile/interactive)
(em-unix-test/compile/pipeline, em-unix-test/compile/subcommand): Fix
indentation.
(em-unix-test/cat/file-output): New test.
* 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 | 101 ++++++++++++++++++++++--------
lisp/eshell/esh-var.el | 7 +--
test/lisp/eshell/em-unix-tests.el | 37 +++++++----
8 files changed, 139 insertions(+), 78 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..9edfe363b8e 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -112,10 +112,30 @@ eshell-error-handle
(defcustom eshell-print-queue-size 5
"The size of the print queue, for doing buffered printing.
-This is basically a speed enhancement, to avoid blocking the Lisp code
-from executing while Emacs is redisplaying."
+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.
+Larger values for this option will generally result in faster execution
+by reducing the overhead associated with each print operation, but will
+increase the time it takes to see any progress in the output; smaller
+values will do the reverse."
+ :type 'integer
+ :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 +480,69 @@ 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."
- (eshell-flush -1))
+ (declare (obsolete #'eshell-with-buffered-print "30.1"))
+ (setq eshell-print-queue nil
+ eshell-print-queue-size 0))
+
+(defun eshell-flush (&optional redisplay-now)
+ "Flush out any text that has been queued for printing.
+When printing interactively, this will call `redisplay' every
+`eshell-buffered-print-redisplay-throttle' seconds so that the user can
+see the progress. If REDISPLAY-NOW is non-nil, call `redisplay' for
+interactive output even if the throttle would otherwise prevent it."
+ (when eshell-print-queue
+ (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
+ (or redisplay-now
+ (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))))
+ "A buffered print -- *for strings only*.
+When the buffer exceeds `eshell-buffered-print-size' in characters, this
+will flush it using `eshell-flush' (which see)."
+ (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.
+Within BODY, call `eshell-buffered-print' to perform output."
+ (declare (indent 0))
+ `(unwind-protect
+ (let ((eshell-print-queue nil)
+ (eshell-print-queue-size 0)
+ (eshell--next-redisplay-time
+ (when (and eshell-buffered-print-redisplay-throttle
+ (eshell-interactive-output-p))
+ (time-add (current-time)
+ eshell-buffered-print-redisplay-throttle))))
+ ,@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."
diff --git a/test/lisp/eshell/em-unix-tests.el b/test/lisp/eshell/em-unix-tests.el
index a92c7d3f80a..2ee42c81333 100644
--- a/test/lisp/eshell/em-unix-tests.el
+++ b/test/lisp/eshell/em-unix-tests.el
@@ -26,10 +26,12 @@
(require 'ert)
(require 'em-unix)
+(eval-and-compile
+ (defvar this-directory (file-name-directory
+ (or load-file-name default-directory))))
+
(require 'eshell-tests-helpers
- (expand-file-name "eshell-tests-helpers"
- (file-name-directory (or load-file-name
- default-directory))))
+ (expand-file-name "eshell-tests-helpers" this-directory))
;;; Tests:
@@ -37,11 +39,11 @@ em-unix-test/compile/interactive
"Check that `eshell/compile' opens a compilation buffer interactively."
(skip-unless (executable-find "echo"))
(with-temp-eshell
- (eshell-match-command-output "compile echo hello"
- "#<buffer \\*compilation\\*>")
- (with-current-buffer "*compilation*"
- (forward-line 3)
- (should (looking-at "echo hello")))))
+ (eshell-match-command-output "compile echo hello"
+ "#<buffer \\*compilation\\*>")
+ (with-current-buffer "*compilation*"
+ (forward-line 3)
+ (should (looking-at "echo hello")))))
(ert-deftest em-unix-test/compile/noninteractive ()
"Check that `eshell/compile' writes to stdout noninteractively."
@@ -54,15 +56,26 @@ em-unix-test/compile/pipeline
(skip-unless (and (executable-find "echo")
(executable-find "cat")))
(with-temp-eshell
- (eshell-match-command-output "compile echo hello | *cat"
- "\\`hello\n")))
+ (eshell-match-command-output "compile echo hello | *cat"
+ "\\`hello\n")))
(ert-deftest em-unix-test/compile/subcommand ()
"Check that `eshell/compile' writes to stdout from a subcommand."
(skip-unless (and (executable-find "echo")
(executable-find "cat")))
(with-temp-eshell
- (eshell-match-command-output "echo ${compile echo hello}"
- "\\`hello\n")))
+ (eshell-match-command-output "echo ${compile echo hello}"
+ "\\`hello\n")))
+
+(ert-deftest em-unix-test/cat/file-output ()
+ "Check that `eshell/cat' can print a file's contents."
+ (with-temp-eshell
+ (let* ((this-file (expand-file-name "em-unix-tests.el" this-directory))
+ (contents (save-current-buffer
+ (find-file this-file)
+ (buffer-string))))
+ (eshell-match-command-output
+ (format "cat '%s'" (string-replace "'" "''" this-file))
+ (concat (regexp-quote contents))))))
;; em-unix-tests.el ends here
--
2.25.1
[-- Attachment #3: 0002-Improve-implementations-of-some-Eshell-output-filter.patch --]
[-- Type: text/plain, Size: 7465 bytes --]
From b8380e89d2163e096e8ebe558ac526f4ba4ed880 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-05 3:50 ` Jim Porter
@ 2024-06-05 12:06 ` Eli Zaretskii
2024-06-05 16:42 ` Jim Porter
0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2024-06-05 12:06 UTC (permalink / raw)
To: Jim Porter; +Cc: 71355, stefankangas
> Date: Tue, 4 Jun 2024 20:50:52 -0700
> From: Jim Porter <jporterbugs@gmail.com>
>
> On 6/4/2024 6:55 PM, Jim Porter wrote:
> > On 6/4/2024 2:52 PM, Stefan Kangas wrote:
> >> Could the docstring be expanded to explain what a user can expect to
> >> happen if they increase or decrease this value?
> >
> > Sure, that makes sense. Essentially, smaller values will be slower, but
> > may update faster (subject to the redisplay throttling), whereas larger
> > values are the opposite.
>
> I've expanded this docstring and a few others, plus added a simple test
> to make sure Eshell's built-in "cat" still works.
Is 2K indeed the optimal size? It is about 25 80-column lines, which
is quite a lot. "Normal" shells send output to the screen in smaller
chunks. How about 128 instead? or maybe some small multiple of the
line length, like 1 or 2?
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-05 12:06 ` Eli Zaretskii
@ 2024-06-05 16:42 ` Jim Porter
2024-06-05 16:51 ` Eli Zaretskii
0 siblings, 1 reply; 22+ messages in thread
From: Jim Porter @ 2024-06-05 16:42 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 71355, stefankangas
On 6/5/2024 5:06 AM, Eli Zaretskii wrote:
> Is 2K indeed the optimal size? It is about 25 80-column lines, which
> is quite a lot. "Normal" shells send output to the screen in smaller
> chunks. How about 128 instead? or maybe some small multiple of the
> line length, like 1 or 2?
Yes, I believe 2k is the optimal size, or close to it. Trying a value of
128 results in basically no change in performance from baseline. That
makes sense to me, since 128 is actually fairly close to the old value
for this buffering (which was five *lines*[1]; the old code measured
this differently).
In starting on this work, I compared to the amount of data that I get
each time a child process filter is called; that's normally 4095 bytes
in my tests. Then I started a bit lower (512) and gradually doubled the
buffer size until I was able to get most of the performance benefit.
A lot of this improvement does come from reducing the number of times
that we call 'eshell-output-filter-functions', so if we made those
functions much faster, or throttled them somewhere else, then we could
get a large perf improvement while using a small buffer size. However,
even if I remove everything from 'eshell-output-filter-functions', a
larger buffer is *still* faster (0.2s for 2048 vs 0.5s for 128). For
comparison, the same command in an ordinary shell takes about 0.1s, so
that's my ultimate target.
Note that this buffered output code is really only used when Eshell will
output a (relatively) large block of text all at once from a Lisp
command. External processes and most ordinary Lisp code won't use this,
since you have to use the special functions for buffering your writes.
[1] Well, 5 calls to the print function, but most callers printed a line
at a time.
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-05 16:42 ` Jim Porter
@ 2024-06-05 16:51 ` Eli Zaretskii
2024-06-05 17:35 ` Jim Porter
0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2024-06-05 16:51 UTC (permalink / raw)
To: Jim Porter; +Cc: 71355, stefankangas
> Date: Wed, 5 Jun 2024 09:42:43 -0700
> Cc: 71355@debbugs.gnu.org, stefankangas@gmail.com
> From: Jim Porter <jporterbugs@gmail.com>
>
> On 6/5/2024 5:06 AM, Eli Zaretskii wrote:
> > Is 2K indeed the optimal size? It is about 25 80-column lines, which
> > is quite a lot. "Normal" shells send output to the screen in smaller
> > chunks. How about 128 instead? or maybe some small multiple of the
> > line length, like 1 or 2?
>
> Yes, I believe 2k is the optimal size, or close to it. Trying a value of
> 128 results in basically no change in performance from baseline. That
> makes sense to me, since 128 is actually fairly close to the old value
> for this buffering (which was five *lines*[1]; the old code measured
> this differently).
That's strange, because I see no output at all until all of it is
available and showsn, and I thought you said the same in your OP?
> In starting on this work, I compared to the amount of data that I get
> each time a child process filter is called; that's normally 4095 bytes
> in my tests. Then I started a bit lower (512) and gradually doubled the
> buffer size until I was able to get most of the performance benefit.
So why do I see only all of it, in one go? I see this both on Windows
and on GNU/Linux, btw.
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-05 16:51 ` Eli Zaretskii
@ 2024-06-05 17:35 ` Jim Porter
2024-06-05 17:57 ` Eli Zaretskii
0 siblings, 1 reply; 22+ messages in thread
From: Jim Porter @ 2024-06-05 17:35 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 71355, stefankangas
On 6/5/2024 9:51 AM, Eli Zaretskii wrote:
>> Date: Wed, 5 Jun 2024 09:42:43 -0700
>> Cc: 71355@debbugs.gnu.org, stefankangas@gmail.com
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> On 6/5/2024 5:06 AM, Eli Zaretskii wrote:
>>> Is 2K indeed the optimal size? It is about 25 80-column lines, which
>>> is quite a lot. "Normal" shells send output to the screen in smaller
>>> chunks. How about 128 instead? or maybe some small multiple of the
>>> line length, like 1 or 2?
>>
>> Yes, I believe 2k is the optimal size, or close to it. Trying a value of
>> 128 results in basically no change in performance from baseline. That
>> makes sense to me, since 128 is actually fairly close to the old value
>> for this buffering (which was five *lines*[1]; the old code measured
>> this differently).
>
> That's strange, because I see no output at all until all of it is
> available and showsn, and I thought you said the same in your OP?
Yes, without my patch that's expected. When I talk about changes in
performance, I mean the total time to complete the command, as measured
by, e.g. "time cat config.log".
Here's what's happening: all of the output in 'eshell/cat' occurs in a
loop, periodically calling 'eshell-interactive-print' (how often it
calls this depends on the buffering settings). That runs the functions
in 'eshell-output-filter-functions', which can be fairly expensive. So
one way to make output faster would be to optimize those functions,
which I did in my second patch. However, a larger buffer size is still
faster even when there are no output filter functions, due to other
overheads in the code. So I think even if we could make
'eshell-output-filter-functions' all very cheap, it's worth increasing
the buffer size.
In addition to this, the performance improvements I made allowed me to
add in the extra work of redisplaying periodically when using this
buffered output scheme. That's all new in my patch, and previously you'd
have to wait until the command was finished to see any output. From
Emacs's perspective, everything in 'eshell/cat' is synchronous, so I
needed to manually trigger the redisplay (or do some other sorcery to
hand control back to the command loop).
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-05 17:35 ` Jim Porter
@ 2024-06-05 17:57 ` Eli Zaretskii
2024-06-05 18:47 ` Jim Porter
0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2024-06-05 17:57 UTC (permalink / raw)
To: Jim Porter; +Cc: 71355, stefankangas
> Date: Wed, 5 Jun 2024 10:35:08 -0700
> Cc: 71355@debbugs.gnu.org, stefankangas@gmail.com
> From: Jim Porter <jporterbugs@gmail.com>
>
> On 6/5/2024 9:51 AM, Eli Zaretskii wrote:
> >> Date: Wed, 5 Jun 2024 09:42:43 -0700
> >> Cc: 71355@debbugs.gnu.org, stefankangas@gmail.com
> >> From: Jim Porter <jporterbugs@gmail.com>
> >>
> >> On 6/5/2024 5:06 AM, Eli Zaretskii wrote:
> >>> Is 2K indeed the optimal size? It is about 25 80-column lines, which
> >>> is quite a lot. "Normal" shells send output to the screen in smaller
> >>> chunks. How about 128 instead? or maybe some small multiple of the
> >>> line length, like 1 or 2?
> >>
> >> Yes, I believe 2k is the optimal size, or close to it. Trying a value of
> >> 128 results in basically no change in performance from baseline. That
> >> makes sense to me, since 128 is actually fairly close to the old value
> >> for this buffering (which was five *lines*[1]; the old code measured
> >> this differently).
> >
> > That's strange, because I see no output at all until all of it is
> > available and showsn, and I thought you said the same in your OP?
>
> Yes, without my patch that's expected. When I talk about changes in
> performance, I mean the total time to complete the command, as measured
> by, e.g. "time cat config.log".
>
> Here's what's happening: all of the output in 'eshell/cat' occurs in a
> loop, periodically calling 'eshell-interactive-print' (how often it
> calls this depends on the buffering settings). That runs the functions
> in 'eshell-output-filter-functions', which can be fairly expensive. So
> one way to make output faster would be to optimize those functions,
> which I did in my second patch. However, a larger buffer size is still
> faster even when there are no output filter functions, due to other
> overheads in the code. So I think even if we could make
> 'eshell-output-filter-functions' all very cheap, it's worth increasing
> the buffer size.
>
> In addition to this, the performance improvements I made allowed me to
> add in the extra work of redisplaying periodically when using this
> buffered output scheme. That's all new in my patch, and previously you'd
> have to wait until the command was finished to see any output. From
> Emacs's perspective, everything in 'eshell/cat' is synchronous, so I
> needed to manually trigger the redisplay (or do some other sorcery to
> hand control back to the command loop).
I think we are miscommunicating. I wasn't talking about performance,
I was talking about the fact that I don't see text delivered to the
screen in chunks. You said that the current code sends text to the
screen in chunks of 5 lines, and that therefore using the value 128 is
almost the same. But at least part of your patch calls redisplay
after each chunk (AFAIU), something that is not done with the current
code. So I expect the effect to be a difference in behavior, whereby
test appears on the screen in chunks, and the user does not need to
wait till all of it is sent before he/she sees anything at all
displayed.
I hope I've succeeded to explain myself now.
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-05 17:57 ` Eli Zaretskii
@ 2024-06-05 18:47 ` Jim Porter
2024-06-05 18:58 ` Eli Zaretskii
0 siblings, 1 reply; 22+ messages in thread
From: Jim Porter @ 2024-06-05 18:47 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 71355, stefankangas
On 6/5/2024 10:57 AM, Eli Zaretskii wrote:
> I think we are miscommunicating. I wasn't talking about performance,
> I was talking about the fact that I don't see text delivered to the
> screen in chunks. You said that the current code sends text to the
> screen in chunks of 5 lines, and that therefore using the value 128 is
> almost the same.
You had asked about changing the buffer size; in my initial explanation,
I was just trying to explain my reasoning for why a larger buffer size
improves the overall execution time. So I was only talking about the
performance (the redisplay changes are mostly independent from the
buffer size setting, since I throttle the redisplays by time).
To be more precise though: the current code writes text into the
*buffer* in chunks of 5 lines. (That text will end up on the screen at
the next redisplay, whenever that is.) With my patch, if I set the
buffer size to 128, it will write text into the buffer every 128 chars,
which results in a similar number of write operations as before the
patch. Since those write operations are what makes the current code
slow, a 128-char buffer with my patch is similarly slow.
> But at least part of your patch calls redisplay
> after each chunk (AFAIU), something that is not done with the current
> code. So I expect the effect to be a difference in behavior, whereby
> test appears on the screen in chunks, and the user does not need to
> wait till all of it is sent before he/she sees anything at all
> displayed.
Correct. The redisplay logic is a new behavior, and not *directly* a
part of the performance improvements I made by increasing the buffer
size. On the contrary, I added the redisplay logic because the buffer
size improvements made the total execution time so much better that I
felt Eshell could now afford the extra cost of redisplaying periodically
for these commands.
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-05 18:47 ` Jim Porter
@ 2024-06-05 18:58 ` Eli Zaretskii
2024-06-05 20:07 ` Jim Porter
0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2024-06-05 18:58 UTC (permalink / raw)
To: Jim Porter; +Cc: 71355, stefankangas
> Date: Wed, 5 Jun 2024 11:47:40 -0700
> Cc: 71355@debbugs.gnu.org, stefankangas@gmail.com
> From: Jim Porter <jporterbugs@gmail.com>
>
> > But at least part of your patch calls redisplay
> > after each chunk (AFAIU), something that is not done with the current
> > code. So I expect the effect to be a difference in behavior, whereby
> > test appears on the screen in chunks, and the user does not need to
> > wait till all of it is sent before he/she sees anything at all
> > displayed.
>
> Correct. The redisplay logic is a new behavior, and not *directly* a
> part of the performance improvements I made by increasing the buffer
> size. On the contrary, I added the redisplay logic because the buffer
> size improvements made the total execution time so much better that I
> felt Eshell could now afford the extra cost of redisplaying periodically
> for these commands.
So now that we agree about this aspect, I ask again: wouldn't it make
sense to show the text to the user in smaller chunks? 2K characters
is 2 dozen lines, and I expect users to be somewhat unhappy about
being presented the text in such large chunks. That's now what they
see when invoking 'cat' from the shell prompt outside Emacs.
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-05 18:58 ` Eli Zaretskii
@ 2024-06-05 20:07 ` Jim Porter
2024-06-06 4:43 ` Eli Zaretskii
0 siblings, 1 reply; 22+ messages in thread
From: Jim Porter @ 2024-06-05 20:07 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 71355, stefankangas
On 6/5/2024 11:58 AM, Eli Zaretskii wrote:
> So now that we agree about this aspect, I ask again: wouldn't it make
> sense to show the text to the user in smaller chunks? 2K characters
> is 2 dozen lines, and I expect users to be somewhat unhappy about
> being presented the text in such large chunks. That's now what they
> see when invoking 'cat' from the shell prompt outside Emacs.
Ok, I see what you mean. I think the thing users would be unhappy about
is "long" periods of time between display updates. (If we flush and/or
redisplay faster than the user's monitor refreshes, those updates are
wasted.)
For the kinds of output that Eshell's buffered-print is designed for, we
can get the text we want to print very quickly, so even with a buffer
size of 2048, we flush more than 60 times a second (testing with "cat"
and "ls" on a spinny disk). In situations where the buffering caused
unacceptable delays, a command would either a) not use buffered output
or b) manually flush at opportune times. I'm not sure those would come
up in practice though.
Ultimately, in the cases where Eshell does buffered-printing now, the
thing that limits the user seeing updates is the redisplay throttle, not
the buffer size.
A smarter version of 'eshell-buffered-print' could flush before the
buffer was full if enough time has passed, but that would add complexity
without a lot of immediate benefit. (For example, would we set up a
timer to flush? I'm not sure how that would interact with the rest of
this code, which is all synchronous.)
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-05 20:07 ` Jim Porter
@ 2024-06-06 4:43 ` Eli Zaretskii
2024-06-06 18:02 ` Jim Porter
0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2024-06-06 4:43 UTC (permalink / raw)
To: Jim Porter; +Cc: 71355, stefankangas
> Date: Wed, 5 Jun 2024 13:07:48 -0700
> Cc: 71355@debbugs.gnu.org, stefankangas@gmail.com
> From: Jim Porter <jporterbugs@gmail.com>
>
> On 6/5/2024 11:58 AM, Eli Zaretskii wrote:
> > So now that we agree about this aspect, I ask again: wouldn't it make
> > sense to show the text to the user in smaller chunks? 2K characters
> > is 2 dozen lines, and I expect users to be somewhat unhappy about
> > being presented the text in such large chunks. That's now what they
> > see when invoking 'cat' from the shell prompt outside Emacs.
>
> Ok, I see what you mean. I think the thing users would be unhappy about
> is "long" periods of time between display updates. (If we flush and/or
> redisplay faster than the user's monitor refreshes, those updates are
> wasted.)
>
> For the kinds of output that Eshell's buffered-print is designed for, we
> can get the text we want to print very quickly, so even with a buffer
> size of 2048, we flush more than 60 times a second (testing with "cat"
> and "ls" on a spinny disk). In situations where the buffering caused
> unacceptable delays, a command would either a) not use buffered output
> or b) manually flush at opportune times. I'm not sure those would come
> up in practice though.
>
> Ultimately, in the cases where Eshell does buffered-printing now, the
> thing that limits the user seeing updates is the redisplay throttle, not
> the buffer size.
>
> A smarter version of 'eshell-buffered-print' could flush before the
> buffer was full if enough time has passed, but that would add complexity
> without a lot of immediate benefit. (For example, would we set up a
> timer to flush? I'm not sure how that would interact with the rest of
> this code, which is all synchronous.)
My main point is that 'cat' is used to show the contents of a file to
the user, so the assumption is that the user _wants_ to see that
stuff. So having the stuff flash before user's eyes in an instant is
not necessarily the best idea, even though it is faster, and thus
performance-wise we win.
If the above is agreed, and you still think 2K characters is the best
default value, I'm fine with that.
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-05 1:55 ` Jim Porter
2024-06-05 3:50 ` Jim Porter
@ 2024-06-06 9:20 ` Stefan Kangas
2024-06-06 18:04 ` Jim Porter
2024-06-06 23:14 ` Stefan Kangas
2 siblings, 1 reply; 22+ messages in thread
From: Stefan Kangas @ 2024-06-06 9:20 UTC (permalink / raw)
To: Jim Porter, 71355
Jim Porter <jporterbugs@gmail.com> writes:
> If anyone has time to test, I'd be interested to see if the results are
> similarly good on other platforms (I'm testing on x86_64 GNU/Linux), but
> I imagine they're comparable.
0. emacs -Q
1. M-x eshell RET
2. time cat config.log RET
I did three attempts:
Before your patch: 4.520 4.687 4.609 secs
With your patch: 0.958 0.968 1.048 secs
In GNU Emacs 30.0.50 (build 3, aarch64-apple-darwin23.5.0, NS
appkit-2487.60 Version 14.5 (Build 23F79)) of 2024-06-06 built on
Stefans-MacBook-Pro.local
Repository revision: a525cfb3af0c49c5c64e8af548ab23d086348fed
Repository branch: master
Windowing system distributor 'Apple', version 10.3.2487
System Description: macOS 14.5
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-06 4:43 ` Eli Zaretskii
@ 2024-06-06 18:02 ` Jim Porter
2024-06-08 4:25 ` Jim Porter
0 siblings, 1 reply; 22+ messages in thread
From: Jim Porter @ 2024-06-06 18:02 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 71355, stefankangas
On 6/5/2024 9:43 PM, Eli Zaretskii wrote:
> My main point is that 'cat' is used to show the contents of a file to
> the user, so the assumption is that the user _wants_ to see that
> stuff. So having the stuff flash before user's eyes in an instant is
> not necessarily the best idea, even though it is faster, and thus
> performance-wise we win.
For "cat" specifically, I think what we want is just to finish as fast
as we can so that Eshell hands control back to the Emacs command loop,
and then the user can start examining the output. (For example, by
pressing C-c C-r to go to the beginning of the output, or using the
"Smart" module[1].)
> If the above is agreed, and you still think 2K characters is the best
> default value, I'm fine with that.
Agreed. I'd definitely like to improve the usability for dealing with
long-running commands or ones that output a lot of text in Eshell. I
think that's a separate task though. (For example, the text flashing
before the user's eyes also happens when running the external
/usr/bin/cat, and I haven't touched that code path here. A fix for that
behavior would go elsewhere so that both the internal and external cats
could benefit.)
[1] I don't use the Smart module so I don't know a ton about it, but it
avoids scrolling the output as it comes in and stays at the beginning.
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-06 9:20 ` Stefan Kangas
@ 2024-06-06 18:04 ` Jim Porter
0 siblings, 0 replies; 22+ messages in thread
From: Jim Porter @ 2024-06-06 18:04 UTC (permalink / raw)
To: Stefan Kangas, 71355
On 6/6/2024 2:20 AM, Stefan Kangas wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
>
>> If anyone has time to test, I'd be interested to see if the results are
>> similarly good on other platforms (I'm testing on x86_64 GNU/Linux), but
>> I imagine they're comparable.
>
> 0. emacs -Q
> 1. M-x eshell RET
> 2. time cat config.log RET
>
> I did three attempts:
>
> Before your patch: 4.520 4.687 4.609 secs
> With your patch: 0.958 0.968 1.048 secs
Thanks. Those differences aren't quite as stark as in my tests, but 4.5x
faster is still pretty substantial, so it's good to see this improves
things on other systems too.
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-05 1:55 ` Jim Porter
2024-06-05 3:50 ` Jim Porter
2024-06-06 9:20 ` Stefan Kangas
@ 2024-06-06 23:14 ` Stefan Kangas
2024-06-07 0:09 ` Jim Porter
2 siblings, 1 reply; 22+ messages in thread
From: Stefan Kangas @ 2024-06-06 23:14 UTC (permalink / raw)
To: Jim Porter, 71355
Jim Porter <jporterbugs@gmail.com> writes:
>>> +(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."
>>
>> How did you decide on this value?
>
> Basically, I tried a few different command invocations that normally
> take a while (the main ones being "cat config.log" and "ls -Al
> /usr/bin") and scaled up the value by factors of two until I stopped
> seeing any major perf improvements. (Larger values are faster still, but
> not by a whole lot.)
FWIW, I've experimented a bit on my machine, and I see the following
using the command "time cat config.log" from an initially empty eshell
buffer:
| eshell-buffered-print-size | secs | |
|----------------------------+------------+-------|
| 256 | 1.922 secs | 1 |
| 512 | 1.413 secs | 0.74 |
| 1024 | 1.065 secs | 0.55 |
| 2048 | 0.996 secs | 0.52 |
| 4096 | 0.860 secs | 0.45 |
| 8192 | 0.835 secs | 0.43 |
| 16384 | 0.829 secs | 0.43 |
To me, these numbers seem to suggest that, at least on this system,
there is a sweet spot around 4096, but 2048 admittedly does already get
us most of the way there. However, going above 8192 doesn't lead to any
appreciable speedup. This is on a fast M2 machine; it would be
interesting to see some experiments on slower machines as well.
I'm assuming that we don't want to set it to some arbitrarily large
number, but do we expect any adverse affects from choosing a slightly
higher value? If not, is there a case to be made for choosing 4096 as
the default?
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-06 23:14 ` Stefan Kangas
@ 2024-06-07 0:09 ` Jim Porter
2024-06-07 8:51 ` Stefan Kangas
0 siblings, 1 reply; 22+ messages in thread
From: Jim Porter @ 2024-06-07 0:09 UTC (permalink / raw)
To: Stefan Kangas, 71355
On 6/6/2024 4:14 PM, Stefan Kangas wrote:
> FWIW, I've experimented a bit on my machine, and I see the following
> using the command "time cat config.log" from an initially empty eshell
> buffer:
>
> | eshell-buffered-print-size | secs | |
> |----------------------------+------------+-------|
> | 256 | 1.922 secs | 1 |
> | 512 | 1.413 secs | 0.74 |
> | 1024 | 1.065 secs | 0.55 |
> | 2048 | 0.996 secs | 0.52 |
> | 4096 | 0.860 secs | 0.45 |
> | 8192 | 0.835 secs | 0.43 |
> | 16384 | 0.829 secs | 0.43 |
>
> To me, these numbers seem to suggest that, at least on this system,
> there is a sweet spot around 4096, but 2048 admittedly does already get
> us most of the way there. However, going above 8192 doesn't lead to any
> appreciable speedup. This is on a fast M2 machine; it would be
> interesting to see some experiments on slower machines as well.
>
> I'm assuming that we don't want to set it to some arbitrarily large
> number, but do we expect any adverse affects from choosing a slightly
> higher value? If not, is there a case to be made for choosing 4096 as
> the default?
This is consistent with what I saw, but I thought it best to err on the
side of smallness for the buffer size, just to ensure that we flush the
buffer faster than the redisplay rate (defaulting to 40Hz), even on
slower machines. Since the difference between 2048 and 4096 is small, I
figured it wasn't too high a price to pay.
In your tests, just like mine, the difference between 1024 and 2048 is
also small, but I opted for the larger 2048 because it produced a
more-noticeable improvement when running `time ls -Al /usr/bin`.
That said, if we wanted to use a buffer size of 4096, I don't think that
would cause any major issues. (At least on my system, when running an
external command in Eshell, it writes 4095 bytes at a time, since that's
how much data we get at once from the process filter. As far as I'm
aware, it's been that way forever.)
On some level, I think any value between about 1024 and 4096 is probably
somewhat arbitrary. They'd likely all be fine in most cases, and have
much better performance than what we have today.
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-07 0:09 ` Jim Porter
@ 2024-06-07 8:51 ` Stefan Kangas
0 siblings, 0 replies; 22+ messages in thread
From: Stefan Kangas @ 2024-06-07 8:51 UTC (permalink / raw)
To: Jim Porter, 71355
Jim Porter <jporterbugs@gmail.com> writes:
> This is consistent with what I saw, but I thought it best to err on the
> side of smallness for the buffer size, just to ensure that we flush the
> buffer faster than the redisplay rate (defaulting to 40Hz), even on
> slower machines. Since the difference between 2048 and 4096 is small, I
> figured it wasn't too high a price to pay.
Makes sense to me, thanks.
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-06 18:02 ` Jim Porter
@ 2024-06-08 4:25 ` Jim Porter
2024-06-08 7:33 ` Stefan Kangas
0 siblings, 1 reply; 22+ messages in thread
From: Jim Porter @ 2024-06-08 4:25 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 71355, stefankangas
[-- Attachment #1: Type: text/plain, Size: 436 bytes --]
Whoops, one of the intermediate changes I'd made while working on these
patches broke a bunch of the regression tests, so here's the fixed
version. I also renamed a few of the new variables to be clearer that
they're for internal use only.
Since it sounds like all the other concerns have been addressed, I'll
probably merge this in the next day or so. (This is the last non-bugfix
change I'd like to land for Eshell in Emacs 30.)
[-- Attachment #2: 0001-Be-more-efficient-when-buffering-output-in-Eshell.patch --]
[-- Type: text/plain, Size: 17643 bytes --]
From 8da40485bab3131577cbacd6f560c8d831e247d3 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
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, eshell-print-queue,
eshell-print-queue-count): Make obsolete in favor of...
(eshell-buffered-print-size, eshell--buffered-print-queue)
(eshell--buffered-print-current-size): ... these.
(eshell-buffered-print-redisplay-throttle): New user option.
(eshell--buffered-print-next-redisplay): New variable.
(eshell-init-print-buffer): Make obsolete.
(eshell-flush): Add new REDISPLAY-NOW argument in favor of CLEAR (which
only 'eshell-init-print-buffer' should have used).
(eshell-buffered-print): Compare queued output length to
'eshell--buffered-print-current-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'.
* test/lisp/eshell/em-unix-tests.el (em-unix-test/compile/interactive)
(em-unix-test/compile/pipeline, em-unix-test/compile/subcommand): Fix
indentation.
(em-unix-test/cat/file-output): New test.
* 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 | 104 +++++++++++++++++++++++-------
lisp/eshell/esh-var.el | 7 +-
test/lisp/eshell/em-unix-tests.el | 37 +++++++----
8 files changed, 143 insertions(+), 77 deletions(-)
diff --git a/etc/NEWS b/etc/NEWS
index d6a8fa7122b..2349cc0cacb 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -963,6 +963,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 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.
+
+++
*** 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 0fe177d4c60..9de9cc4509a 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -112,10 +112,30 @@ eshell-error-handle
(defcustom eshell-print-queue-size 5
"The size of the print queue, for doing buffered printing.
-This is basically a speed enhancement, to avoid blocking the Lisp code
-from executing while Emacs is redisplaying."
+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.
+Larger values for this option will generally result in faster execution
+by reducing the overhead associated with each print operation, but will
+increase the time it takes to see any progress in the output; smaller
+values will do the reverse."
+ :type 'integer
+ :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 +480,74 @@ eshell-interactive-output-p
(equal (caar (aref handles eshell-error-handle)) '(t)))
(equal (caar (aref handles index)) '(t)))))
+(defvar eshell--buffered-print-queue nil)
+(defvar eshell--buffered-print-current-size nil)
+(defvar eshell--buffered-print-next-redisplay nil)
+
(defvar eshell-print-queue nil)
+(make-obsolete-variable 'eshell-print-queue
+ 'eshell--buffered-print-queue "30.1")
(defvar eshell-print-queue-count -1)
+(make-obsolete-variable 'eshell-print-queue-count
+ 'eshell--buffered-print-current-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."
- (eshell-flush -1))
+ (declare (obsolete #'eshell-with-buffered-print "30.1"))
+ (setq eshell--buffered-print-queue nil
+ eshell--buffered-print-current-size 0))
+
+(defun eshell-flush (&optional redisplay-now)
+ "Flush out any text that has been queued for printing.
+When printing interactively, this will call `redisplay' every
+`eshell-buffered-print-redisplay-throttle' seconds so that the user can
+see the progress. If REDISPLAY-NOW is non-nil, call `redisplay' for
+interactive output even if the throttle would otherwise prevent it."
+ (ignore
+ (when eshell--buffered-print-queue
+ (eshell-print (apply #'concat eshell--buffered-print-queue))
+ ;; When printing interactively (see `eshell-with-buffered-print'),
+ ;; periodically redisplay so the user can see some progress.
+ (when (and eshell--buffered-print-next-redisplay
+ (or redisplay-now
+ (time-less-p eshell--buffered-print-next-redisplay
+ (current-time))))
+ (redisplay)
+ (setq eshell--buffered-print-next-redisplay
+ (time-add eshell--buffered-print-next-redisplay
+ eshell-buffered-print-redisplay-throttle)))
+ (setq eshell--buffered-print-queue nil
+ eshell--buffered-print-current-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))))
+ "A buffered print -- *for strings only*.
+When the buffer exceeds `eshell-buffered-print-size' in characters, this
+will flush it using `eshell-flush' (which see)."
+ (setq eshell--buffered-print-queue
+ (nconc eshell--buffered-print-queue strings))
+ (cl-incf eshell--buffered-print-current-size
+ (apply #'+ (mapcar #'length strings)))
+ (when (> eshell--buffered-print-current-size eshell-buffered-print-size)
+ (eshell-flush)))
+
+(defmacro eshell-with-buffered-print (&rest body)
+ "Initialize buffered printing for Eshell, and then evaluate BODY.
+Within BODY, call `eshell-buffered-print' to perform output."
+ (declare (indent 0))
+ `(let ((eshell--buffered-print-queue nil)
+ (eshell--buffered-print-current-size 0)
+ (eshell--buffered-print-next-redisplay
+ (when (and eshell-buffered-print-redisplay-throttle
+ (eshell-interactive-output-p))
+ (time-add (current-time)
+ eshell-buffered-print-redisplay-throttle))))
+ (unwind-protect
+ ,@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."
diff --git a/test/lisp/eshell/em-unix-tests.el b/test/lisp/eshell/em-unix-tests.el
index a92c7d3f80a..2ee42c81333 100644
--- a/test/lisp/eshell/em-unix-tests.el
+++ b/test/lisp/eshell/em-unix-tests.el
@@ -26,10 +26,12 @@
(require 'ert)
(require 'em-unix)
+(eval-and-compile
+ (defvar this-directory (file-name-directory
+ (or load-file-name default-directory))))
+
(require 'eshell-tests-helpers
- (expand-file-name "eshell-tests-helpers"
- (file-name-directory (or load-file-name
- default-directory))))
+ (expand-file-name "eshell-tests-helpers" this-directory))
;;; Tests:
@@ -37,11 +39,11 @@ em-unix-test/compile/interactive
"Check that `eshell/compile' opens a compilation buffer interactively."
(skip-unless (executable-find "echo"))
(with-temp-eshell
- (eshell-match-command-output "compile echo hello"
- "#<buffer \\*compilation\\*>")
- (with-current-buffer "*compilation*"
- (forward-line 3)
- (should (looking-at "echo hello")))))
+ (eshell-match-command-output "compile echo hello"
+ "#<buffer \\*compilation\\*>")
+ (with-current-buffer "*compilation*"
+ (forward-line 3)
+ (should (looking-at "echo hello")))))
(ert-deftest em-unix-test/compile/noninteractive ()
"Check that `eshell/compile' writes to stdout noninteractively."
@@ -54,15 +56,26 @@ em-unix-test/compile/pipeline
(skip-unless (and (executable-find "echo")
(executable-find "cat")))
(with-temp-eshell
- (eshell-match-command-output "compile echo hello | *cat"
- "\\`hello\n")))
+ (eshell-match-command-output "compile echo hello | *cat"
+ "\\`hello\n")))
(ert-deftest em-unix-test/compile/subcommand ()
"Check that `eshell/compile' writes to stdout from a subcommand."
(skip-unless (and (executable-find "echo")
(executable-find "cat")))
(with-temp-eshell
- (eshell-match-command-output "echo ${compile echo hello}"
- "\\`hello\n")))
+ (eshell-match-command-output "echo ${compile echo hello}"
+ "\\`hello\n")))
+
+(ert-deftest em-unix-test/cat/file-output ()
+ "Check that `eshell/cat' can print a file's contents."
+ (with-temp-eshell
+ (let* ((this-file (expand-file-name "em-unix-tests.el" this-directory))
+ (contents (save-current-buffer
+ (find-file this-file)
+ (buffer-string))))
+ (eshell-match-command-output
+ (format "cat '%s'" (string-replace "'" "''" this-file))
+ (concat (regexp-quote contents))))))
;; em-unix-tests.el ends here
--
2.25.1
[-- Attachment #3: 0002-Improve-implementations-of-some-Eshell-output-filter.patch --]
[-- Type: text/plain, Size: 7465 bytes --]
From d797cbe58628166c0f1ca1bfce2c92a2b59a7b49 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
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
^ permalink raw reply related [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-08 4:25 ` Jim Porter
@ 2024-06-08 7:33 ` Stefan Kangas
2024-06-08 19:43 ` Jim Porter
0 siblings, 1 reply; 22+ messages in thread
From: Stefan Kangas @ 2024-06-08 7:33 UTC (permalink / raw)
To: Jim Porter, Eli Zaretskii; +Cc: 71355
Jim Porter <jporterbugs@gmail.com> writes:
> Since it sounds like all the other concerns have been addressed, I'll
> probably merge this in the next day or so. (This is the last non-bugfix
> change I'd like to land for Eshell in Emacs 30.)
Sounds good to me.
^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell
2024-06-08 7:33 ` Stefan Kangas
@ 2024-06-08 19:43 ` Jim Porter
0 siblings, 0 replies; 22+ messages in thread
From: Jim Porter @ 2024-06-08 19:43 UTC (permalink / raw)
To: Stefan Kangas, Eli Zaretskii; +Cc: 71355-done
On 6/8/2024 12:33 AM, Stefan Kangas wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
>
>> Since it sounds like all the other concerns have been addressed, I'll
>> probably merge this in the next day or so. (This is the last non-bugfix
>> change I'd like to land for Eshell in Emacs 30.)
>
> Sounds good to me.
I took one last look after sleeping on it, and everything seems correct
to me, so I've now merged this to the master branch as 15f515c7a37.
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2024-06-08 19:43 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-04 5:36 bug#71355: 30.0.50; [PATCH] Improve performance of buffered output in Eshell Jim Porter
2024-06-04 21:52 ` Stefan Kangas
2024-06-05 1:55 ` Jim Porter
2024-06-05 3:50 ` Jim Porter
2024-06-05 12:06 ` Eli Zaretskii
2024-06-05 16:42 ` Jim Porter
2024-06-05 16:51 ` Eli Zaretskii
2024-06-05 17:35 ` Jim Porter
2024-06-05 17:57 ` Eli Zaretskii
2024-06-05 18:47 ` Jim Porter
2024-06-05 18:58 ` Eli Zaretskii
2024-06-05 20:07 ` Jim Porter
2024-06-06 4:43 ` Eli Zaretskii
2024-06-06 18:02 ` Jim Porter
2024-06-08 4:25 ` Jim Porter
2024-06-08 7:33 ` Stefan Kangas
2024-06-08 19:43 ` Jim Porter
2024-06-06 9:20 ` Stefan Kangas
2024-06-06 18:04 ` Jim Porter
2024-06-06 23:14 ` Stefan Kangas
2024-06-07 0:09 ` Jim Porter
2024-06-07 8:51 ` Stefan Kangas
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.