bug#38457: 27.0.50; dabbrev-expand regression due to message change

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Stephen Berman
0. emacs -Q
1. M-! te M-/

=> It takes about 4 seconds on my machine for the expansion "text" to
appear, during which the minibuffer displays "[Scanning for
dabbrevs...100%]" and then "[Scanning for dabbrevs...done]".  On builds
without this regression, the expansion is virtually instantaneous and no
message is seen in the minibuffer.

Git bisect pinpoints the following change:

aa89c84e00d8dc85100e6fedab7631c415e6364d is the first bad commit
commit aa89c84e00d8dc85100e6fedab7631c415e6364d
Author: Juri Linkov <[hidden email]>
Date:   Wed Nov 27 01:43:49 2019 +0200

    message uses minibuffer-message in the active minibuffer (bug#17272 bug#19064)

    * doc/lispref/display.texi (Displaying Messages): Explain the
    behavior of using minibuffer-message if the minibuffer is active.

    * src/editfns.c (Fmessage_in_echo_area): New function with body
    copied from Fmessage.
    (Fmessage): Call minibuffer-message in the active minibuffer,
    otherwise call Fmessage_in_echo_area.
    (message-in-echo-area): New variable.

    * lisp/isearch.el (isearch--momentary-message, isearch-message):
    * lisp/minibuffer.el (minibuffer-message, minibuffer-completion-help):
    Use 'message-in-echo-area' instead of 'message' where necessary.

    * lisp/autorevert.el (auto-revert-handler):
    * lisp/man.el (Man-bgproc-sentinel):
    * lisp/subr.el (do-after-load-evaluation):
    Revert recent changes that replaced 'message' with 'minibuffer-message'.
    This is not needed anymore since 'message' uses 'minibuffer-message'
    in the active minibuffer.


In GNU Emacs 27.0.50 (build 27, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
 of 2019-12-01 built on strobe-lfs84
Repository revision: 9f2145f42daab13aed5cf89fdb6a7c5579819ec0
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
System Description: Linux From Scratch

Configured using:
 'configure --with-cairo 'CFLAGS=-Og -g3'
 PKG_CONFIG_PATH=/opt/qt5/lib/pkgconfig'

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



Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Eli Zaretskii
> From: Stephen Berman <[hidden email]>
> Date: Mon, 02 Dec 2019 12:06:55 +0100
>
> 0. emacs -Q
> 1. M-! te M-/
>
> => It takes about 4 seconds on my machine for the expansion "text" to
> appear, during which the minibuffer displays "[Scanning for
> dabbrevs...100%]" and then "[Scanning for dabbrevs...done]".  On builds
> without this regression, the expansion is virtually instantaneous and no
> message is seen in the minibuffer.
>
> Git bisect pinpoints the following change:
>
> aa89c84e00d8dc85100e6fedab7631c415e6364d is the first bad commit
> commit aa89c84e00d8dc85100e6fedab7631c415e6364d
> Author: Juri Linkov <[hidden email]>
> Date:   Wed Nov 27 01:43:49 2019 +0200
>
>     message uses minibuffer-message in the active minibuffer (bug#17272 bug#19064)
>
>     * doc/lispref/display.texi (Displaying Messages): Explain the
>     behavior of using minibuffer-message if the minibuffer is active.
>
>     * src/editfns.c (Fmessage_in_echo_area): New function with body
>     copied from Fmessage.
>     (Fmessage): Call minibuffer-message in the active minibuffer,
>     otherwise call Fmessage_in_echo_area.
>     (message-in-echo-area): New variable.
>
>     * lisp/isearch.el (isearch--momentary-message, isearch-message):
>     * lisp/minibuffer.el (minibuffer-message, minibuffer-completion-help):
>     Use 'message-in-echo-area' instead of 'message' where necessary.
>
>     * lisp/autorevert.el (auto-revert-handler):
>     * lisp/man.el (Man-bgproc-sentinel):
>     * lisp/subr.el (do-after-load-evaluation):
>     Revert recent changes that replaced 'message' with 'minibuffer-message'.
>     This is not needed anymore since 'message' uses 'minibuffer-message'
>     in the active minibuffer.

I guess the new message-in-echo-area confuses dabbrev.el because it
switches buffers, and dabbrev.el has special logic for that, which
triggers the message, and the subsequent wait.



Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Juri Linkov-2
In reply to this post by Stephen Berman
> 0. emacs -Q
> 1. M-! te M-/
>
> => It takes about 4 seconds on my machine for the expansion "text" to
> appear, during which the minibuffer displays "[Scanning for
> dabbrevs...100%]" and then "[Scanning for dabbrevs...done]".  On builds
> without this regression, the expansion is virtually instantaneous and no
> message is seen in the minibuffer.

This is because of the current limitation of minibuffer-message.
It uses sit-for to wait for 2 seconds per every message.
This should be fixed by using the timer, so there will be no delays
anymore:


diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index a7bdde478f..3febdeb174 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -702,6 +702,9 @@ minibuffer
 (defvar minibuffer-message-properties nil
   "Text properties added to the text shown by `minibuffer-message'.")
 
+(defvar minibuffer-message-timer nil)
+(defvar minibuffer-message-overlay nil)
+
 (defun minibuffer-message (message &rest args)
   "Temporarily display MESSAGE at the end of the minibuffer.
 The text is displayed for `minibuffer-message-timeout' seconds,
@@ -732,24 +735,23 @@ minibuffer-message
                 ;; Don't overwrite the face properties the caller has set
                 (text-properties-at 0 message))
       (setq message (apply #'propertize message minibuffer-message-properties)))
-    (let ((ol (make-overlay (point-max) (point-max) nil t t))
-          ;; A quit during sit-for normally only interrupts the sit-for,
-          ;; but since minibuffer-message is used at the end of a command,
-          ;; at a time when the command has virtually finished already, a C-g
-          ;; should really cause an abort-recursive-edit instead (i.e. as if
-          ;; the C-g had been typed at top-level).  Binding inhibit-quit here
-          ;; is an attempt to get that behavior.
-          (inhibit-quit t))
-      (unwind-protect
-          (progn
-            (unless (zerop (length message))
-              ;; The current C cursor code doesn't know to use the overlay's
-              ;; marker's stickiness to figure out whether to place the cursor
-              ;; before or after the string, so let's spoon-feed it the pos.
-              (put-text-property 0 1 'cursor t message))
-            (overlay-put ol 'after-string message)
-            (sit-for (or minibuffer-message-timeout 1000000)))
-        (delete-overlay ol)))))
+
+    (when (timerp minibuffer-message-timer)
+      (cancel-timer minibuffer-message-timer))
+    (when (overlayp minibuffer-message-overlay)
+      (delete-overlay minibuffer-message-overlay))
+    (setq minibuffer-message-overlay
+          (make-overlay (point-max) (point-max) nil t t))
+    (setq minibuffer-message-timer
+          (run-with-timer (or minibuffer-message-timeout 1) nil
+                          (lambda () (when (overlayp minibuffer-message-overlay)
+                                       (delete-overlay minibuffer-message-overlay)))))
+    (unless (zerop (length message))
+      ;; The current C cursor code doesn't know to use the overlay's
+      ;; marker's stickiness to figure out whether to place the cursor
+      ;; before or after the string, so let's spoon-feed it the pos.
+      (put-text-property 0 1 'cursor t message))
+    (overlay-put minibuffer-message-overlay 'after-string message)))
 
 (defun minibuffer-completion-contents ()
   "Return the user input in a minibuffer before point as a string.
Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Stephen Berman
On Tue, 03 Dec 2019 01:00:40 +0200 Juri Linkov <[hidden email]> wrote:

>> 0. emacs -Q
>> 1. M-! te M-/
>>
>> => It takes about 4 seconds on my machine for the expansion "text" to
>> appear, during which the minibuffer displays "[Scanning for
>> dabbrevs...100%]" and then "[Scanning for dabbrevs...done]".  On builds
>> without this regression, the expansion is virtually instantaneous and no
>> message is seen in the minibuffer.
>
> This is because of the current limitation of minibuffer-message.
> It uses sit-for to wait for 2 seconds per every message.
> This should be fixed by using the timer, so there will be no delays
> anymore:

I confirm that this patch eliminates the dabbrev-expand delay.
Simultaneously with the expansion, the message "[Scanning for
dabbrevs...done]" is displayed, which is uninformative and hence a bit
annoying, but otherwise seems harmless.  Thanks for the fix.

Steve Berman



Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Eli Zaretskii
In reply to this post by Juri Linkov-2
> From: Juri Linkov <[hidden email]>
> Date: Tue, 03 Dec 2019 01:00:40 +0200
> Cc: [hidden email]
>
> > 0. emacs -Q
> > 1. M-! te M-/
> >
> > => It takes about 4 seconds on my machine for the expansion "text" to
> > appear, during which the minibuffer displays "[Scanning for
> > dabbrevs...100%]" and then "[Scanning for dabbrevs...done]".  On builds
> > without this regression, the expansion is virtually instantaneous and no
> > message is seen in the minibuffer.
>
> This is because of the current limitation of minibuffer-message.
> It uses sit-for to wait for 2 seconds per every message.
> This should be fixed by using the timer, so there will be no delays
> anymore:

Please don't make this change, or any other changes in this stuff that
significantly modify the internals of these basic APIs.  I'm that
close to ask to revert the entire message-in-echo-area change, because
it's already too invasive and runs a high risk of disrupting too many
commands.  We don't want the pretest of Emacs 27.1 to last forever for
these reasons.

So please try to find a simpler fix for this problem, one that doesn't
involve a timer.  If it isn't possible, let's indeed consider removing
the message-in-echo-area changes for now.

Sorry for that, but we should keep in mind that Emacs 27.1 pretest is
coming soon, and any deep changes in low-level infrastructure used all
over the place should wait for the next release.

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Juri Linkov-2
> So please try to find a simpler fix for this problem, one that doesn't
> involve a timer.

This is the simplest and the safest solution.  I wanted to implement it
earlier but had no time before.  I see no problems with it.



Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Eli Zaretskii
> From: Juri Linkov <[hidden email]>
> Cc: [hidden email],  [hidden email]
> Date: Wed, 04 Dec 2019 01:44:57 +0200
>
> > So please try to find a simpler fix for this problem, one that doesn't
> > involve a timer.
>
> This is the simplest and the safest solution.

The simplest solution would be to prevent dabbrev-expand from
displaying the message in the first place; there are conditions for
that which evidently the new implementation somehow triggers.

> I wanted to implement it earlier but had no time before.  I see no
> problems with it.

Let's not rock the boat too much so close to the release cycle because
of a very minor problem which caused these changes.  You may not see
any problems with it now, but I'm certain we will bump into numerous
problems very soon if we make such significant changes in this
infrastructure, which is used everywhere in Emacs.



Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Juri Linkov-2
>> I wanted to implement it earlier but had no time before.  I see no
>> problems with it.
>
> Let's not rock the boat too much so close to the release cycle because
> of a very minor problem which caused these changes.  You may not see
> any problems with it now, but I'm certain we will bump into numerous
> problems very soon if we make such significant changes in this
> infrastructure, which is used everywhere in Emacs.

There may be a misunderstanding here.  I was talking about the
changes for development in master, not for the release branch.
If you think it would be too risky to release with these changes,
then fine, these changes could be removed in the release branch.



Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Eli Zaretskii
> From: Juri Linkov <[hidden email]>
> Cc: [hidden email],  [hidden email]
> Date: Thu, 05 Dec 2019 01:16:34 +0200
>
> > Let's not rock the boat too much so close to the release cycle because
> > of a very minor problem which caused these changes.  You may not see
> > any problems with it now, but I'm certain we will bump into numerous
> > problems very soon if we make such significant changes in this
> > infrastructure, which is used everywhere in Emacs.
>
> There may be a misunderstanding here.  I was talking about the
> changes for development in master, not for the release branch.
> If you think it would be too risky to release with these changes,
> then fine, these changes could be removed in the release branch.

The master branch will become the release branch very soon.  If you
want to wait with these changes after the emacs-27 branch is cut, it's
fine by me, but we need to solve the issue with dabbrev-expand for the
release branch as well.

That is why I suggest to find a simpler, safer solution for that issue
now, and if you insist on using a timer (which I personally consider
not a good idea), then do this after the release branch is cut.

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Kévin Le Gouguec
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> writes:

>> From: Juri Linkov <[hidden email]>
>>
>> > So please try to find a simpler fix for this problem, one that doesn't
>> > involve a timer.
>>
>> This is the simplest and the safest solution.
>
> The simplest solution would be to prevent dabbrev-expand from
> displaying the message in the first place; there are conditions for
> that which evidently the new implementation somehow triggers.
>
>> I wanted to implement it earlier but had no time before.  I see no
>> problems with it.
>
> Let's not rock the boat too much so close to the release cycle because
> of a very minor problem which caused these changes.  You may not see
> any problems with it now, but I'm certain we will bump into numerous
> problems very soon if we make such significant changes in this
> infrastructure, which is used everywhere in Emacs.

Mmm, as you say, the message infrastructure is used everywhere in Emacs,
and it has received significant changes in aa89c84e00d.

I think the regression Stephen noticed is more widespread than just
dabbrev-expand: e.g. TRAMP is also affected[1].  Does it make sense to
address each symptom individually?  Were dabbrev, TRAMP, and possibly
others wrong to assume that they could call (message …) without causing
2-second delays?

NB: I am not saying that Juri's initial patch is the simplest nor the
safest.  It just seems to me that aa89c84e00d has already "rocked the
boat" quite a bit, and addressing this specific bug report by tweaking
dabbrev-expand will only take care of the tip of the iceberg, to
continue the nautical metaphor…


[1] C-x C-f /ssh:localhost: RET

    ⇒ This hangs for 10 seconds unless the user furiously spams some
    command (e.g. C-n), as each of the following message takes 2 seconds
    to disappear:

    > Tramp: Opening connection for localhost using ssh...
    > Tramp: Sending command ‘exec ssh   -o ControlMaster=auto -o ControlPath='tramp.%C' -o ControlPersist=no -e none localhost’
    > Tramp: Waiting for prompts from remote shell...done
    > Tramp: Found remote shell prompt on ‘localhost’
    > Tramp: Opening connection for localhost using ssh...done

    I haven't reported this as a separate issue yet, because I am
    assuming that both bugs are caused by recent changes in message and
    should be fixed there…



Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Eli Zaretskii
> From: Kévin Le Gouguec <[hidden email]>
> Cc: Juri Linkov <[hidden email]>,  [hidden email],
>   [hidden email]
> Date: Thu, 05 Dec 2019 16:24:08 +0100
>
> Mmm, as you say, the message infrastructure is used everywhere in Emacs,
> and it has received significant changes in aa89c84e00d.
>
> I think the regression Stephen noticed is more widespread than just
> dabbrev-expand: e.g. TRAMP is also affected[1].  Does it make sense to
> address each symptom individually?

I didn't say individually, I said I would like to see a simpler
change, one that doesn't yet again change the infrastructure
significantly.

For example, to fix the dabbrev case we could bind
minibuffer-message-timeout to zero in progress-reporter-do-update.

> Were dabbrev, TRAMP, and possibly others wrong to assume that they
> could call (message …) without causing 2-second delays?

No.



Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Juri Linkov-2
In reply to this post by Kévin Le Gouguec
>>> > So please try to find a simpler fix for this problem, one that doesn't
>>> > involve a timer.
>>>
>>> This is the simplest and the safest solution.
>>
>> The simplest solution would be to prevent dabbrev-expand from
>> displaying the message in the first place; there are conditions for
>> that which evidently the new implementation somehow triggers.
>>
>>> I wanted to implement it earlier but had no time before.  I see no
>>> problems with it.
>>
>> Let's not rock the boat too much so close to the release cycle because
>> of a very minor problem which caused these changes.  You may not see
>> any problems with it now, but I'm certain we will bump into numerous
>> problems very soon if we make such significant changes in this
>> infrastructure, which is used everywhere in Emacs.
>
> Mmm, as you say, the message infrastructure is used everywhere in Emacs,
> and it has received significant changes in aa89c84e00d.
>
> I think the regression Stephen noticed is more widespread than just
> dabbrev-expand: e.g. TRAMP is also affected[1].

I know that TRAMP is affected.  I had no time to address this before, sorry,
but now this is fixed by the previous patch I sent 3 days ago that uses a timer.



Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Juri Linkov-2
In reply to this post by Eli Zaretskii
>> > Let's not rock the boat too much so close to the release cycle because
>> > of a very minor problem which caused these changes.  You may not see
>> > any problems with it now, but I'm certain we will bump into numerous
>> > problems very soon if we make such significant changes in this
>> > infrastructure, which is used everywhere in Emacs.
>>
>> There may be a misunderstanding here.  I was talking about the
>> changes for development in master, not for the release branch.
>> If you think it would be too risky to release with these changes,
>> then fine, these changes could be removed in the release branch.
>
> The master branch will become the release branch very soon.  If you
> want to wait with these changes after the emacs-27 branch is cut, it's
> fine by me, but we need to solve the issue with dabbrev-expand for the
> release branch as well.
>
> That is why I suggest to find a simpler, safer solution for that issue
> now, and if you insist on using a timer (which I personally consider
> not a good idea), then do this after the release branch is cut.

Using a timer is the right thing to do, there is no simpler solution,
it solves all problems.



Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Eli Zaretskii
In reply to this post by Juri Linkov-2
> From: Juri Linkov <[hidden email]>
> Cc: Eli Zaretskii <[hidden email]>,  [hidden email],
>   [hidden email]
> Date: Fri, 06 Dec 2019 02:06:30 +0200
>
> > I think the regression Stephen noticed is more widespread than just
> > dabbrev-expand: e.g. TRAMP is also affected[1].
>
> I know that TRAMP is affected.  I had no time to address this before, sorry,
> but now this is fixed by the previous patch I sent 3 days ago that uses a timer.

We need to fix this without a timer, and we need to fix this before
the emacs-27 branch is cut.

One possibility is for 'message' to bind 'minibuffer-message-timeout'
to zero when it calls 'minibuffer-message'.  Would this work in all
the use cases discovered to day?

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Eli Zaretskii
In reply to this post by Juri Linkov-2
> From: Juri Linkov <[hidden email]>
> Cc: [hidden email],  [hidden email]
> Date: Fri, 06 Dec 2019 02:10:20 +0200
>
> > That is why I suggest to find a simpler, safer solution for that issue
> > now, and if you insist on using a timer (which I personally consider
> > not a good idea), then do this after the release branch is cut.
>
> Using a timer is the right thing to do, there is no simpler solution,
> it solves all problems.

Sorry, I object to installing a timer-based solution at this time.  It
is too radical and is likely to cause numerous unintended consequences
all over the place.

I just suggested another possible solution: bind
'minibuffer-message-timeout' to zero when calling 'minibuffer-message'
from 'message'.  Would that work, or will it have some unwanted side
effects?



Reply | Threaded
Open this post in threaded view
|

bug#38457: 27.0.50; dabbrev-expand regression due to message change

Kévin Le Gouguec
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> writes:

> One possibility is for 'message' to bind 'minibuffer-message-timeout'
> to zero when it calls 'minibuffer-message'.  Would this work in all
> the use cases discovered to day?

FWIW the following patch does remove the delay before {the dabbrev
expansion shows up,TRAMP proceeds with each step}:


diff --git a/src/editfns.c b/src/editfns.c
index 72a9cdba7a..3ee00fcc1b 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -2895,6 +2895,8 @@ accent (\\=`) and apostrophe (\\=') are special in the format; see
 
       /* Avoid possible recursion.  */
       specbind (Qmessage_in_echo_area, Qt);
+      /* Do not delay callers (bug#38457).  */
+      specbind (intern ("minibuffer-message-timeout"), INT_TO_INTEGER (0));
 
       record_unwind_current_buffer ();
       Fset_buffer (Fwindow_buffer (Fold_selected_window ()));


Though that makes each message flicker briefly and disappear before the
user has a chance to read it.