bug#41640: 28.0.50; shell startup very slow when init file is used

bug#41640: 28.0.50; shell startup very slow when init file is used

[Jan Synacek]

$ cat ~/.emacs.d/init_bash.sh
export PS1="emacs> "

$ emacs -Q

<inside Emacs, run 'M-x shell'>

It takes almost 2 seconds before the shell buffer appears and Emacs is
frozen in the mean time. Without the init file, the shell buffer appears
instantly.


In GNU Emacs 28.0.50 (build 5, x86_64-pc-linux-gnu, GTK+ Version 3.24.20, cairo version 1.16.0)
 of 2020-06-01 built on jsynacek-ntb.brq.redhat.com
Repository revision: f92925864613035c2e627862433112b12cf0d6dd
Repository branch: master
Windowing system distributor 'Fedora Project', version 11.0.12008000
System Description: Fedora 32 (Workstation Edition)

Recent messages:
Loading /home/jsynacek/.emacs.d/config.el (source)...
Loading /home/jsynacek/.emacs.d/recentf...done
Cleaning up the recentf list...done (0 removed)
Loading /home/jsynacek/.emacs.d/config.el (source)...done
Loaded /home/jsynacek/.emacs.d/config.el
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --prefix=/home/jsynacek/pkgs/emacs'

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF
ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS PDUMPER LCMS2
GMP

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  recentf-mode: t
  show-paren-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny rfc822 mml mml-sec epa
derived epg epg-config gnus-util rmail rmail-loaddefs
text-property-search mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils dired dired-loaddefs edmacro kmacro
recentf tree-widget wid-edit paren cl-extra help-mode org ob ob-tangle
ob-ref ob-lob ob-table ob-exp org-macro org-footnote org-src ob-comint
org-pcomplete pcomplete comint ansi-color ring org-list org-faces
org-entities time-date noutline outline easy-mmode org-version
ob-emacs-lisp ob-core ob-eval org-table ol org-keys org-compat advice
org-macs org-loaddefs format-spec find-func cal-menu calendar
cal-loaddefs slime-autoloads w3m-load info package easymenu browse-url
url-handlers url-parse auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json subr-x map url-vars seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads dbusbind inotify lcms2 dynamic-setting
system-font-setting font-render-setting cairo move-toolbar gtk x-toolkit
x multi-tty make-network-process emacs)

Memory information:
((conses 16 115817 14803)
 (symbols 48 13753 1)
 (strings 32 44504 2337)
 (string-bytes 1 1565056)
 (vectors 16 20650)
 (vector-slots 8 234558 11276)
 (floats 8 80 123)
 (intervals 56 217 0)
 (buffers 992 10))

bug#41640: 28.0.50; shell startup very slow when init file is used

[Pip Cet]

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

Jan Synacek <jsynacek@redhat.com> writes:
> It takes almost 2 seconds before the shell buffer appears and Emacs is
> frozen in the mean time. Without the init file, the shell buffer appears
> instantly.

Can you try this patch?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Avoid-1s-sleep-for-before-sending-the-startfile-to-a.patch --]
[-- Type: text/x-diff, Size: 1427 bytes --]

From d5f1df0edfe6f5d4b178d30d78567bec223927ee Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Thu, 4 Jun 2020 21:18:10 +0000
Subject: [PATCH] Avoid 1s sleep-for before sending the startfile to a comint
 process

* lisp/comint.el (comint-exec): Simplify startup file code.
(Bug#41640)
---
 lisp/comint.el | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index ea06f8af87..4354a155c3 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -809,18 +809,10 @@ comint-exec
       (goto-char (point-max))
       (set-marker (process-mark proc) (point))
       ;; Feed it the startfile.
-      (cond (startfile
-	     ;;This is guaranteed to wait long enough
-	     ;;but has bad results if the comint does not prompt at all
-	     ;;	     (while (= size (buffer-size))
-	     ;;	       (sleep-for 1))
-	     ;;I hope 1 second is enough!
-	     (sleep-for 1)
-	     (goto-char (point-max))
-	     (insert-file-contents startfile)
-	     (setq startfile (buffer-substring (point) (point-max)))
-	     (delete-region (point) (point-max))
-	     (comint-send-string proc startfile)))
+      (when startfile
+        (comint-send-string proc (with-temp-buffer
+                                   (insert-file-contents startfile)
+                                   (buffer-string))))
       (run-hooks 'comint-exec-hook)
       buffer)))
 
-- 
2.27.0.rc0

bug#41640: 28.0.50; shell startup very slow when init file is used

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Thu, 04 Jun 2020 21:21:00 +0000
> Cc: 41640@debbugs.gnu.org
> 
> diff --git a/lisp/comint.el b/lisp/comint.el
> index ea06f8af87..4354a155c3 100644
> --- a/lisp/comint.el
> +++ b/lisp/comint.el
> @@ -809,18 +809,10 @@ comint-exec
>        (goto-char (point-max))
>        (set-marker (process-mark proc) (point))
>        ;; Feed it the startfile.
> -      (cond (startfile
> -	     ;;This is guaranteed to wait long enough
> -	     ;;but has bad results if the comint does not prompt at all
> -	     ;;	     (while (= size (buffer-size))
> -	     ;;	       (sleep-for 1))
> -	     ;;I hope 1 second is enough!
> -	     (sleep-for 1)
> -	     (goto-char (point-max))
> -	     (insert-file-contents startfile)
> -	     (setq startfile (buffer-substring (point) (point-max)))
> -	     (delete-region (point) (point-max))
> -	     (comint-send-string proc startfile)))
> +      (when startfile
> +        (comint-send-string proc (with-temp-buffer
> +                                   (insert-file-contents startfile)
> +                                   (buffer-string))))

The code and the comments don't say why we used sleep-for, which your
patch removes.  Did you succeed in understanding what was that for,
and if so, can you describe that reason and the rationale for removing
the sleep?

Thanks.

bug#41640: 28.0.50; shell startup very slow when init file is used

[Pip Cet]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Pip Cet <pipcet@gmail.com>
>> Date: Thu, 04 Jun 2020 21:21:00 +0000
>> Cc: 41640@debbugs.gnu.org
>> 
>> diff --git a/lisp/comint.el b/lisp/comint.el
>> index ea06f8af87..4354a155c3 100644
>> --- a/lisp/comint.el
>> +++ b/lisp/comint.el
>> @@ -809,18 +809,10 @@ comint-exec
>>        (goto-char (point-max))
>>        (set-marker (process-mark proc) (point))
>>        ;; Feed it the startfile.
>> -      (cond (startfile
>> -	     ;;This is guaranteed to wait long enough
>> -	     ;;but has bad results if the comint does not prompt at all
>> -	     ;;	     (while (= size (buffer-size))
>> -	     ;;	       (sleep-for 1))
>> -	     ;;I hope 1 second is enough!
>> -	     (sleep-for 1)
>> -	     (goto-char (point-max))
>> -	     (insert-file-contents startfile)
>> -	     (setq startfile (buffer-substring (point) (point-max)))
>> -	     (delete-region (point) (point-max))
>> -	     (comint-send-string proc startfile)))
>> +      (when startfile
>> +        (comint-send-string proc (with-temp-buffer
>> +                                   (insert-file-contents startfile)
>> +                                   (buffer-string))))
>
> The code and the comments don't say why we used sleep-for, which your
> patch removes.  Did you succeed in understanding what was that for,
> and if so, can you describe that reason and the rationale for removing
> the sleep?

I'll try. (According to git, this code has been here since 1990). The
code inserted the file contents of the startfile at (point-max) in the
comint *output* buffer. I believe the concern was that
insert-file-contents would accept process output, corrupting the start
file, and it's possible this may happen, at least with tramp
handlers. Therefore, to avoid the process output being sent back to the
process, I think the sleep-for was added as a way to ensure the program
was done displaying the prompt.

If the patch helps (it does here), we're going to have to think about
whether to apply it on master, but I'm not that far yet.

bug#41640: 28.0.50; shell startup very slow when init file is used

[Jan Synacek]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Pip Cet <pipcet@gmail.com>
>> Date: Thu, 04 Jun 2020 21:21:00 +0000
>> Cc: 41640@debbugs.gnu.org
>> 
>> diff --git a/lisp/comint.el b/lisp/comint.el
>> index ea06f8af87..4354a155c3 100644
>> --- a/lisp/comint.el
>> +++ b/lisp/comint.el
>> @@ -809,18 +809,10 @@ comint-exec
>>        (goto-char (point-max))
>>        (set-marker (process-mark proc) (point))
>>        ;; Feed it the startfile.
>> -      (cond (startfile
>> -	     ;;This is guaranteed to wait long enough
>> -	     ;;but has bad results if the comint does not prompt at all
>> -	     ;;	     (while (= size (buffer-size))
>> -	     ;;	       (sleep-for 1))
>> -	     ;;I hope 1 second is enough!
>> -	     (sleep-for 1)
>> -	     (goto-char (point-max))
>> -	     (insert-file-contents startfile)
>> -	     (setq startfile (buffer-substring (point) (point-max)))
>> -	     (delete-region (point) (point-max))
>> -	     (comint-send-string proc startfile)))
>> +      (when startfile
>> +        (comint-send-string proc (with-temp-buffer
>> +                                   (insert-file-contents startfile)
>> +                                   (buffer-string))))

This patch removes the slowness.

bug#41640: 28.0.50; shell startup very slow when init file is used

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Cc: jsynacek@redhat.com,  41640@debbugs.gnu.org
> Date: Fri, 05 Jun 2020 08:30:11 +0000
> 
> > The code and the comments don't say why we used sleep-for, which your
> > patch removes.  Did you succeed in understanding what was that for,
> > and if so, can you describe that reason and the rationale for removing
> > the sleep?
> 
> I'll try. (According to git, this code has been here since 1990).

Yes.  But Jim Blandy, who clearly had some problem he tried to solve,
didn't describe the problem itself, just the solution.

> The code inserted the file contents of the startfile at (point-max)
> in the comint *output* buffer. I believe the concern was that
> insert-file-contents would accept process output, corrupting the
> start file, and it's possible this may happen, at least with tramp
> handlers. Therefore, to avoid the process output being sent back to
> the process, I think the sleep-for was added as a way to ensure the
> program was done displaying the prompt.

So you are saying that the wait is there to allow us to receive the
shell's prompt, so that the contents of start file don't mix with that
prompt?  If that is the reason, then I think your changes are indeed
TRT.

Thanks.

bug#41640: 28.0.50; shell startup very slow when init file is used

[Pip Cet]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Pip Cet <pipcet@gmail.com>
>> Cc: jsynacek@redhat.com,  41640@debbugs.gnu.org
>> Date: Fri, 05 Jun 2020 08:30:11 +0000
>> 
>> > The code and the comments don't say why we used sleep-for, which your
>> > patch removes.  Did you succeed in understanding what was that for,
>> > and if so, can you describe that reason and the rationale for removing
>> > the sleep?
>> 
>> I'll try. (According to git, this code has been here since 1990).
>
> Yes.  But Jim Blandy, who clearly had some problem he tried to solve,
> didn't describe the problem itself, just the solution.

Thanks for calling me out on that. Yes, you're right, we don't know why
the code has been like this for 30 years.

>> The code inserted the file contents of the startfile at (point-max)
>> in the comint *output* buffer. I believe the concern was that
>> insert-file-contents would accept process output, corrupting the
>> start file, and it's possible this may happen, at least with tramp
>> handlers. Therefore, to avoid the process output being sent back to
>> the process, I think the sleep-for was added as a way to ensure the
>> program was done displaying the prompt.
>
> So you are saying that the wait is there to allow us to receive the
> shell's prompt, so that the contents of start file don't mix with that
> prompt?

Sorry, I should have been clearer: this is speculation. I'm not prepared
to suggest the patch for master until I've at least tried to reproduce
the original problem using file handlers.

There's also an alternative explanation, which is that the systems
available back then might have discarded input arriving before the
prompt was first written. But, if that's the case today, it's likely to
be done for a reason, and we shouldn't try to prevent it.

bug#41640: 28.0.50; shell startup very slow when init file is used

[Lars Ingebrigtsen]

Pip Cet <pipcet@gmail.com> writes:

> There's also an alternative explanation, which is that the systems
> available back then might have discarded input arriving before the
> prompt was first written. But, if that's the case today, it's likely to
> be done for a reason, and we shouldn't try to prevent it.

The person reporting the bug says that Pip's patch fixes the problem.
There was some discussion about what the original code from the 90s was
trying to fix, and the answer was...  that we don't know.

So I just went ahead and applied the patch.  I tried it here, and it
didn't break anything, but there are many systems out there.  But I
think it's worth a try, so I'm pushing it to Emacs 28.  If it breaks
anything, we should revert it.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no