Re: A whole lotta auto-saving going

Re: A whole lotta auto-saving going

[Aaron Jensen]

> The past few days on the trunk, I'm seeing "Auto-saving...done" a whole
> lot more than before.  Sometimes it's popping up every few characters
> I'm typing, which just seems unhelpful.
>
> I haven't investigated at all why this is happening -- is it immediately
> obvious to somebody what's going on?

I too am seeing this in my emacs config in enh-ruby-mode. If I change
auto-save-interval to 0, it stops. So it appears that the
auto-save-interval is somehow broken.

I wonder if adbb4eacc2a984c0fc0b65ec761368fd9067d6c5 is related?

There was a change to force_auto_save_soon including a deletion of
code that had a comment indicating the code wasn't fully understood at
some point:

-  /* FIXME: What's the relationship between forcing auto-save and adding
-     a buffer-switch event?  */
-  record_asynch_buffer_change ();

Aaron

Re: A whole lotta auto-saving going

[Lars Ingebrigtsen]

Aaron Jensen <aaronjensen@gmail.com> writes:

> I too am seeing this in my emacs config in enh-ruby-mode. If I change
> auto-save-interval to 0, it stops. So it appears that the
> auto-save-interval is somehow broken.
>
> I wonder if adbb4eacc2a984c0fc0b65ec761368fd9067d6c5 is related?
>
> There was a change to force_auto_save_soon including a deletion of
> code that had a comment indicating the code wasn't fully understood at
> some point:
>
> -  /* FIXME: What's the relationship between forcing auto-save and adding
> -     a buffer-switch event?  */
> -  record_asynch_buffer_change ();

Ah, that does seem kinda likely -- Stefan added to the CCs.

I'm still seeing this issue, but I haven't yet found time to try to
debug it.  Instead I've switched auto-saving off in larger buffers (it
somehow seems like it's triggering more in those buffers).

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

Re: A whole lotta auto-saving going

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Wed, 6 Jan 2021 16:05:52 -0600
> Cc: Lars Ingebrigtsen <larsi@gnus.org>
> 
> > The past few days on the trunk, I'm seeing "Auto-saving...done" a whole
> > lot more than before.  Sometimes it's popping up every few characters
> > I'm typing, which just seems unhelpful.
> >
> > I haven't investigated at all why this is happening -- is it immediately
> > obvious to somebody what's going on?
> 
> I too am seeing this in my emacs config in enh-ruby-mode. If I change
> auto-save-interval to 0, it stops. So it appears that the
> auto-save-interval is somehow broken.

I suggest to put a breakpoint in Fdo_auto_save and see what invokes it
and why.  First, which code invokes it?  Next, if it's indeed this
place:

  if (commandflag != 0 && commandflag != -2
      && auto_save_interval > 0
      && num_nonmacro_input_events - last_auto_save > max (auto_save_interval, 20)
      && !detect_input_pending_run_timers (0))
    {
      Fdo_auto_save (auto_save_no_message ? Qt : Qnil, Qnil);
      /* Hooks can actually change some buffers in auto save.  */
      redisplay ();
    }

then why does it happen?  Do we fail to update last_auto_save? or do
we increment num_nonmacro_input_events too quickly for some reason?

I presume that this doesn't happen in "emacs -Q", because I was unable
to reproduce it.  Then maybe some customizations cause this, directly
or indirectly.  For example, maybe some timers or subprocesses cause
this.

Re: A whole lotta auto-saving going

[Stefan Monnier]

>> -  /* FIXME: What's the relationship between forcing auto-save and adding
>> -     a buffer-switch event?  */
>> -  record_asynch_buffer_change ();
>
> Ah, that does seem kinda likely -- Stefan added to the CCs.
>
> I'm still seeing this issue, but I haven't yet found time to try to
> debug it.  Instead I've switched auto-saving off in larger buffers (it
> somehow seems like it's triggering more in those buffers).

I'm afraid I can't see how that code could increase the
auto-save frequency.  But keep in mind that I never understood how that
code can interact with auto-save anyway,


        Stefan

Re: A whole lotta auto-saving going

[Aaron Jensen]

On Thu, Jan 7, 2021 at 9:21 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> >> -  /* FIXME: What's the relationship between forcing auto-save and adding
> >> -     a buffer-switch event?  */
> >> -  record_asynch_buffer_change ();
> >
> > Ah, that does seem kinda likely -- Stefan added to the CCs.
>
> I'm afraid I can't see how that code could increase the
> auto-save frequency.  But keep in mind that I never understood how that
> code can interact with auto-save anyway,

Just to confirm for now, the referenced commit,
adbb4eacc2a984c0fc0b65ec761368fd9067d6c5, does introduce the problem
for me.

I haven't been able to debug any further yet, but I'll try and do that.

Aaron

Re: A whole lotta auto-saving going

[Aaron Jensen]

On Fri, Jan 8, 2021 at 8:05 AM Aaron Jensen <aaronjensen@gmail.com> wrote:
>
> I haven't been able to debug any further yet, but I'll try and do that.

Here's a repro from emacs -Q:


(require 'package)
(let* ((no-ssl (and (memq system-type '(windows-nt ms-dos))
                    (not (gnutls-available-p))))
       (url (concat (if no-ssl "http" "https") "://melpa.org/packages/")))
  (add-to-list 'package-archives (cons "melpa" url) t))
(package-initialize)
(package-install 'enh-ruby-mode)
(enh-ruby-mode)
(auto-save-mode)

Then start typing, it should flicker "Auto-saving...done" in the echo area

Aaron

Re: A whole lotta auto-saving going

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I suggest to put a breakpoint in Fdo_auto_save and see what invokes it
> and why.

Will do.

> I presume that this doesn't happen in "emacs -Q", because I was unable
> to reproduce it.  Then maybe some customizations cause this, directly
> or indirectly.  For example, maybe some timers or subprocesses cause
> this.

Yeah, I've not been able to reproduce it from -Q, either.  Timers
triggering it seems to be likely, but it also seems to perhaps have
something to do with the number of buffers with unsaved changes?  For
instance, when I'm typing away in this buffer, it was auto-saving every
half-dozen characters, or whenever I stopped typing for half a second.
But then I `C-u C-x s', and then it stopped being as aggressive...

Now it's just auto-saving whenever I stop typing for a second or so...

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

Re: A whole lotta auto-saving going

[Lars Ingebrigtsen]

Aaron Jensen <aaronjensen@gmail.com> writes:

> Here's a repro from emacs -Q:
>
> (require 'package)
> (let* ((no-ssl (and (memq system-type '(windows-nt ms-dos))
>                     (not (gnutls-available-p))))
>        (url (concat (if no-ssl "http" "https") "://melpa.org/packages/")))
>   (add-to-list 'package-archives (cons "melpa" url) t))
> (package-initialize)
> (package-install 'enh-ruby-mode)
> (enh-ruby-mode)
> (auto-save-mode)
>
> Then start typing, it should flicker "Auto-saving...done" in the echo area

Do you have a repro that doesn't involve installing outside packages?
:-)  That would help a lot.

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

Re: A whole lotta auto-saving going

[Aaron Jensen]

On Sun, Jan 10, 2021 at 5:10 AM Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
> Aaron Jensen <aaronjensen@gmail.com> writes:
>
> > Here's a repro from emacs -Q:
> >
> > (require 'package)
> > (let* ((no-ssl (and (memq system-type '(windows-nt ms-dos))
> >                     (not (gnutls-available-p))))
> >        (url (concat (if no-ssl "http" "https") "://melpa.org/packages/")))
> >   (add-to-list 'package-archives (cons "melpa" url) t))
> > (package-initialize)
> > (package-install 'enh-ruby-mode)
> > (enh-ruby-mode)
> > (auto-save-mode)
> >
> > Then start typing, it should flicker "Auto-saving...done" in the echo area
>
> Do you have a repro that doesn't involve installing outside packages?
> :-)  That would help a lot.

No, I don't, though I'm sure one could whittle down the external
dependency to just the piece that causes it. enh-ruby-mode has an
external process that it runs and communicates with in an
after-change-function. I'd start there.

Re: A whole lotta auto-saving going

[Lars Ingebrigtsen]

Aaron Jensen <aaronjensen@gmail.com> writes:

>> Do you have a repro that doesn't involve installing outside packages?
>> :-)  That would help a lot.
>
> No, I don't

I've got one.  From emacs -Q:

M-x shell RET
make -j4 bootstrap RET
C-f /tmp/some.el RET

and then start typing away.  Emacs then autosaves the some.el file every
other keystroke.

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

Re: A whole lotta auto-saving going

[Stefan Monnier]

>> Here's a repro from emacs -Q:
>>
>> (require 'package)
>> (let* ((no-ssl (and (memq system-type '(windows-nt ms-dos))
>>                     (not (gnutls-available-p))))
>>        (url (concat (if no-ssl "http" "https") "://melpa.org/packages/")))
>>   (add-to-list 'package-archives (cons "melpa" url) t))
>> (package-initialize)
>> (package-install 'enh-ruby-mode)
>> (enh-ruby-mode)
>> (auto-save-mode)
>>
>> Then start typing, it should flicker "Auto-saving...done" in the echo area
>
> Do you have a repro that doesn't involve installing outside packages?
> :-)  That would help a lot.

Yes, really, I mean, c'mon!  First you don't send a patch to fix the
problem and then to add insult to injury you taunt us with a recipe that
relies on external code?!

OK, more seriously: thanks for the recipe, I can now reproduce the
problem locally.

Indeed reversing my patch avoids the problem, and the subset of it below
is enough to avoid the problem.

The manifestation of the problem is the following: in reach_char, where
we do

       {
	  Lisp_Object tem0;
	  EMACS_INT timeout = XFIXNAT (Vauto_save_timeout);

	  timeout = min (timeout, MOST_POSITIVE_FIXNUM / delay_level * 4);
	  timeout = delay_level * timeout / 4;
	  ptrdiff_t count1 = SPECPDL_INDEX ();
	  save_getcjmp (save_jump);
	  record_unwind_protect_ptr (restore_getcjmp, save_jump);
	  restore_getcjmp (local_getcjmp);
	  tem0 = sit_for (make_fixnum (timeout), 1, 1);
	  unbind_to (count1, Qnil);

	  if (EQ (tem0, Qt)
	      && ! CONSP (Vunread_command_events))
	    {
	      Fdo_auto_save (auto_save_no_message ? Qt : Qnil, Qnil);
	      redisplay ();
	    }
	}

the sit_for ends up returning "immediately" and with a value Qt.
I suspect this is because enh-ruby-mode receives process output during
the sit_for (which is what causes the "immediate" return) and the value
returned is Qt because there indeed no pending input.

sit_for waits with:

    wait_reading_process_output (sec, nsec, reading ? -1 : 1, do_display,
                                 Qnil, NULL, 0);

AFAIK with the patch below the sit_for also returns "immediately" but it
returns Qnil instead of Qt, and I assume this is because there is now
a BUFFER_SWITCH_EVENT in the queue.  If that's indeed what was
happening, then I think that the call to `record_asynch_buffer_change`
might have been a kind of "lucky workaround" and what should have happened
instead is that `wait_reading_process_output` should not return that
quickly (it's called with a 30s timeout argument).

So the way I see it the question is: why does
`wait_reading_process_output` return "immediately" when there's no
pending input and the timeout argument says 30s?


        Stefan



diff --git a/src/keyboard.c b/src/keyboard.c
index 9ee4c4f6d6..24402fa30c 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -3430,7 +3433,8 @@ readable_events (int flags)
 		       && event->ie.part == scroll_bar_handle
 		       && event->ie.modifiers == 0)
 #endif
-		 )
+		  && !((flags & READABLE_EVENTS_FILTER_EVENTS)
+		       && event->kind == BUFFER_SWITCH_EVENT))
 		return 1;
 	      event = next_kbd_event (event);
 	    }
@@ -3581,6 +3585,12 @@ kbd_buffer_store_buffered_event (union buffered_input_event *event,
 	  return;
 	}
     }
+  /* Don't insert two BUFFER_SWITCH_EVENT's in a row.
+     Just ignore the second one.  */
+  else if (event->kind == BUFFER_SWITCH_EVENT
+	   && kbd_fetch_ptr != kbd_store_ptr
+	   && prev_kbd_event (kbd_store_ptr)->kind == BUFFER_SWITCH_EVENT)
+    return;
 
   /* Don't let the very last slot in the buffer become full,
      since that would make the two pointers equal,
@@ -3614,6 +3624,7 @@ kbd_buffer_store_buffered_event (union buffered_input_event *event,
     case ICONIFY_EVENT: ignore_event = Qiconify_frame; break;
     case DEICONIFY_EVENT: ignore_event = Qmake_frame_visible; break;
     case SELECTION_REQUEST_EVENT: ignore_event = Qselection_request; break;
+    case BUFFER_SWITCH_EVENT: ignore_event = Qbuffer_switch; break;
     default: ignore_event = Qnil; break;
     }
 
@@ -3953,6 +3964,7 @@ kbd_buffer_get_event (KBOARD **kbp,
 #ifdef HAVE_XWIDGETS
       case XWIDGET_EVENT:
 #endif
+      case BUFFER_SWITCH_EVENT:
       case SAVE_SESSION_EVENT:
       case NO_EVENT:
       case HELP_EVENT:
@@ -5332,6 +5344,14 @@ make_lispy_event (struct input_event *event)
       return list2 (Qmove_frame, list1 (event->frame_or_window));
 #endif
 
+    case BUFFER_SWITCH_EVENT:
+      {
+	/* The value doesn't matter here; only the type is tested.  */
+	Lisp_Object obj;
+        XSETBUFFER (obj, current_buffer);
+        return obj;
+      }
+
     /* Just discard these, by returning nil.
        With MULTI_KBOARD, these events are used as placeholders
        when we need to randomly delete events from the queue.
@@ -6788,6 +6808,41 @@ get_input_pending (int flags)
   return input_pending;
 }
 
+/* Put a BUFFER_SWITCH_EVENT in the buffer
+   so that read_key_sequence will notice the new current buffer.  */
+
+void
+record_asynch_buffer_change (void)
+{
+  /* We don't need a buffer-switch event unless Emacs is waiting for input.
+     The purpose of the event is to make read_key_sequence look up the
+     keymaps again.  If we aren't in read_key_sequence, we don't need one,
+     and the event could cause trouble by messing up (input-pending-p).
+     Note: Fwaiting_for_user_input_p always returns nil when async
+     subprocesses aren't supported.  */
+  if (!NILP (Fwaiting_for_user_input_p ()))
+    {
+      struct input_event event;
+
+      EVENT_INIT (event);
+      event.kind = BUFFER_SWITCH_EVENT;
+      event.frame_or_window = Qnil;
+      event.arg = Qnil;
+
+      /* Make sure no interrupt happens while storing the event.  */
+#ifdef USABLE_SIGIO
+      if (interrupt_input)
+	kbd_buffer_store_event (&event);
+      else
+#endif
+	{
+	  stop_polling ();
+	  kbd_buffer_store_event (&event);
+	  start_polling ();
+	}
+    }
+}
+
 /* Read any terminal input already buffered up by the system
    into the kbd_buffer, but do not wait.
 
@@ -11521,6 +11576,8 @@ syms_of_keyboard (void)
   /* Menu and tool bar item parts.  */
   DEFSYM (Qmenu_enable, "menu-enable");
 
+  DEFSYM (Qbuffer_switch, "buffer-switch");
+
 #ifdef HAVE_NTGUI
   DEFSYM (Qlanguage_change, "language-change");
   DEFSYM (Qend_session, "end-session");
diff --git a/src/keyboard.h b/src/keyboard.h
index 8bdffaa2bf..74b7d8517f 100644
--- a/src/keyboard.h
+++ b/src/keyboard.h
@@ -446,6 +446,7 @@ #define EVENT_HEAD_KIND(event_head) \
 extern void push_frame_kboard (struct frame *);
 extern void pop_kboard (void);
 extern void temporarily_switch_to_single_kboard (struct frame *);
+extern void record_asynch_buffer_change (void);
 extern void input_poll_signal (int);
 extern void start_polling (void);
 extern void stop_polling (void);
diff --git a/src/process.c b/src/process.c
index 67e930e18f..ee238ab307 100644
--- a/src/process.c
+++ b/src/process.c
@@ -6130,6 +6130,18 @@ read_and_dispose_of_process_output (struct Lisp_Process *p, char *chars,
   /* Restore waiting_for_user_input_p as it was
      when we were called, in case the filter clobbered it.  */
   waiting_for_user_input_p = waiting;
+
+#if 0 /* Call record_asynch_buffer_change unconditionally,
+	 because we might have changed minor modes or other things
+	 that affect key bindings.  */
+  if (! EQ (Fcurrent_buffer (), obuffer)
+      || ! EQ (current_buffer->keymap, okeymap))
+#endif
+    /* But do it only if the caller is actually going to read events.
+       Otherwise there's no need to make him wake up, and it could
+       cause trouble (for example it would make sit_for return).  */
+    if (waiting_for_user_input_p == -1)
+      record_asynch_buffer_change ();
 }
 
 DEFUN ("internal-default-process-filter", Finternal_default_process_filter,
diff --git a/src/termhooks.h b/src/termhooks.h
index 85a47c071b..dd59c66ae2 100644
--- a/src/termhooks.h
+++ b/src/termhooks.h
@@ -159,6 +159,7 @@ #define EMACS_TERMHOOKS_H
   SELECTION_REQUEST_EVENT,	/* Another X client wants a selection from us.
 				   See `struct selection_input_event'.  */
   SELECTION_CLEAR_EVENT,	/* Another X client cleared our selection.  */
+  BUFFER_SWITCH_EVENT,		/* A process filter has switched buffers.  */
   DELETE_WINDOW_EVENT,		/* An X client said "delete this window".  */
 #ifdef HAVE_NTGUI
   END_SESSION_EVENT,		/* The user is logging out or shutting down.  */

Re: A whole lotta auto-saving going

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 10 Jan 2021 16:24:48 +0100
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> M-x shell RET
> make -j4 bootstrap RET
> C-f /tmp/some.el RET
> 
> and then start typing away.  Emacs then autosaves the some.el file every
> other keystroke.

I guess the counter of non-menu events goes up very quickly, because
the subprocess produces a lot of output?

Re: A whole lotta auto-saving going

[Stefan Monnier]

>> M-x shell RET
>> make -j4 bootstrap RET
>> C-f /tmp/some.el RET
>> 
>> and then start typing away.  Emacs then autosaves the some.el file every
>> other keystroke.
>
> I guess the counter of non-menu events goes up very quickly, because
> the subprocess produces a lot of output?

From what I can tell, in the recipe I used, the do-auto-save function is
not called because of the number of non-menu events but because of the
"idle timeout" (and indeed, AFAICT the process output does not generate
any "events").


        Stefan

Re: A whole lotta auto-saving going

[T.V Raman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=gb18030, Size: 439 bytes --]

Tangential (slightly) but given the repro case, perhaps relevant:

Why is buffer-undo-list   not set up to avoid saving undo information in
shell buffers?
AKA, should we have (setq buffer-undo-list t) in shell buffers? Perhaps
even in all comint buffers? I dont think I have had reason to "undo" in
a shell buffer, and even if I did, I dont quite know what that means.

-- 

Thanks,

--Raman
7©4 Id: kg:/m/0285kf1  •0Ü8

Re: A whole lotta auto-saving going

[Aaron Jensen]

On Sun, Jan 10, 2021 at 12:34 PM T.V Raman <raman@google.com> wrote:
>
> Tangential (slightly) but given the repro case, perhaps relevant:
>
> Why is buffer-undo-list   not set up to avoid saving undo information in
> shell buffers?

If I can type in a buffer, I'd want to be able to undo, ideally. Is it
recording undo steps for things that the shell itself emits? That
would seem wrong.

Aaron

Re: A whole lotta auto-saving going

[Stefan Monnier]

> From what I can tell, in the recipe I used, the do-auto-save function is
> not called because of the number of non-menu events but because of the
> "idle timeout" (and indeed, AFAICT the process output does not generate
> any "events").

Apparently, the early exit from `wait_reading_process_output` comes from
the following `break`:

          if (!process_skipped && got_some_output > 0
              && (timeout.tv_sec > 0 || timeout.tv_nsec > 0))
            {
              if (!timespec_valid_p (got_output_end_time))
                break;

Does someone here understanding something of what
`wait_reading_process_output` does and what it is expected to do?

Why does it exit here before the end of the timeout?  IIUC it is
supposed to exit as soon as we got some output from `wait_proc`, but in
this case `wait_proc` is NULL.  Is it also supposed to exit when some
process output arrives?  If so, shouldn't `sit_for` wrap the call to
`wait_reading_process_output` inside a loop to make sure we wait the
whole timeout?


        Stefan

Re: A whole lotta auto-saving going

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  aaronjensen@gmail.com,
>   emacs-devel@gnu.org
> Date: Sun, 10 Jan 2021 23:23:23 -0500
> 
> Apparently, the early exit from `wait_reading_process_output` comes from
> the following `break`:
> 
>           if (!process_skipped && got_some_output > 0
>               && (timeout.tv_sec > 0 || timeout.tv_nsec > 0))
>             {
>               if (!timespec_valid_p (got_output_end_time))
>                 break;
> 
> Does someone here understanding something of what
> `wait_reading_process_output` does and what it is expected to do?

You mean, in general? or in this specific case?

The latter is described by the comment above this fragment.

> Why does it exit here before the end of the timeout?  IIUC it is
> supposed to exit as soon as we got some output from `wait_proc`, but in
> this case `wait_proc` is NULL.  Is it also supposed to exit when some
> process output arrives?  If so, shouldn't `sit_for` wrap the call to
> `wait_reading_process_output` inside a loop to make sure we wait the
> whole timeout?

I think sitting for the entire period is undesirable, since receiving
output from a process might require redisplay.  In that case, waiting
could make Emacs seem unresponsive or busy, whereas it really isn't.

I think a simple solution to this would be to check the time passed
after sit_for returns, and if some of the wait time is left, not call
auto-save.  This would mimic what happened before the offending
changeset.

We could, of course, add a loop inside sit_for, but that function is
called in a few other places, and even in this place it isn't certain
that we never want it to return early upon receiving process output.
So, while adding a loop, and then dealing with whatever further
breakage this will cause could be an exciting project, I'm not sure we
are looking for such an excitement.  One cleanup at a time, okay?

Re: A whole lotta auto-saving going

[Stefan Monnier]

>> Does someone here understanding something of what
>> `wait_reading_process_output` does and what it is expected to do?
> You mean, in general? or in this specific case?

Both.

> The latter is described by the comment above this fragment.

It gives an idea, yes, but not enough to know what is expected in the
specific case that corresponds to how it's called from `sit_for`.

>> Why does it exit here before the end of the timeout?  IIUC it is
>> supposed to exit as soon as we got some output from `wait_proc`, but in
>> this case `wait_proc` is NULL.  Is it also supposed to exit when some
>> process output arrives?  If so, shouldn't `sit_for` wrap the call to
>> `wait_reading_process_output` inside a loop to make sure we wait the
>> whole timeout?
> I think sitting for the entire period is undesirable, since receiving
> output from a process might require redisplay.
> In that case, waiting could make Emacs seem unresponsive or busy,
> whereas it really isn't.

But the `do_display` argument indicates that if redisplay is needed it
can happen without returning from `wait_reading_process_output`.

> I think a simple solution to this would be to check the time passed
> after sit_for returns, and if some of the wait time is left, not call
> auto-save.  This would mimic what happened before the offending
> changeset.

The patch below implements that option.

There's one other call to `sit_for` which can be affected:

	  tem0 = sit_for (Vecho_keystrokes, 1, 1);
	  unbind_to (count, Qnil);
	  if (EQ (tem0, Qt)
	      && ! CONSP (Vunread_command_events))
	    echo_now ();

I believe it's an improvement there as well.


        Stefan


diff --git a/src/dispnew.c b/src/dispnew.c
index 36a6dd8a09..39adbb2229 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -6049,7 +6049,9 @@ DEFUN ("sleep-for", Fsleep_for, Ssleep_for, 1, 2, 0,
    READING is true if reading input.
    If DISPLAY_OPTION is >0 display process output while waiting.
    If DISPLAY_OPTION is >1 perform an initial redisplay before waiting.
-*/
+
+   Returns a boolean Qt if we waited the full time and returns Qnil if the
+   wait was interrupted by incoming process output or keyboard events.  */
 
 Lisp_Object
 sit_for (Lisp_Object timeout, bool reading, int display_option)
@@ -6110,8 +6112,9 @@ sit_for (Lisp_Object timeout, bool reading, int display_option)
   gobble_input ();
 #endif
 
-  wait_reading_process_output (sec, nsec, reading ? -1 : 1, do_display,
-			       Qnil, NULL, 0);
+  int nbytes
+    = wait_reading_process_output (sec, nsec, reading ? -1 : 1, do_display,
+			           Qnil, NULL, 0);
 
   if (reading && curbuf_eq_winbuf)
     /* Timers and process filters/sentinels may have changed the selected
@@ -6120,7 +6123,7 @@ sit_for (Lisp_Object timeout, bool reading, int display_option)
        buffer to start with).  */
     set_buffer_internal (XBUFFER (XWINDOW (selected_window)->contents));
 
-  return detect_input_pending () ? Qnil : Qt;
+  return (nbytes > 0 || detect_input_pending ()) ? Qnil : Qt;
 }
 
 

Re: A whole lotta auto-saving going

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: larsi@gnus.org,  aaronjensen@gmail.com,  emacs-devel@gnu.org
> Date: Mon, 11 Jan 2021 11:00:04 -0500
> 
> > I think sitting for the entire period is undesirable, since receiving
> > output from a process might require redisplay.
> > In that case, waiting could make Emacs seem unresponsive or busy,
> > whereas it really isn't.
> 
> But the `do_display` argument indicates that if redisplay is needed it
> can happen without returning from `wait_reading_process_output`.

Do we get to where that causes redisplay in this case?

> > I think a simple solution to this would be to check the time passed
> > after sit_for returns, and if some of the wait time is left, not call
> > auto-save.  This would mimic what happened before the offending
> > changeset.
> 
> The patch below implements that option.

Thanks.

I see you look at the number of bytes read, but what about the case
that a process exited?

> There's one other call to `sit_for` which can be affected:
> 
> 	  tem0 = sit_for (Vecho_keystrokes, 1, 1);
> 	  unbind_to (count, Qnil);
> 	  if (EQ (tem0, Qt)
> 	      && ! CONSP (Vunread_command_events))
> 	    echo_now ();
> 
> I believe it's an improvement there as well.

Why do you believe that?  It's a different use case, and I don't think
we saw any adverse effects there from the removal of the buffer-switch
"event".  Are there any adverse effects?

Re: A whole lotta auto-saving going

[Stefan Monnier]

>> But the `do_display` argument indicates that if redisplay is needed it
>> can happen without returning from `wait_reading_process_output`.
> Do we get to where that causes redisplay in this case?

I don't know, but we should because the code does pass a true value for
this parameter.

>> > I think a simple solution to this would be to check the time passed
>> > after sit_for returns, and if some of the wait time is left, not call
>> > auto-save.  This would mimic what happened before the offending
>> > changeset.
>> The patch below implements that option.
> Thanks.
> I see you look at the number of bytes read, but what about the case
> that a process exited?

I don't know anything about that case, sorry (i.e. I don't know if it
causes `wait_reading_process_output` to exit early).  So ... let me test it.
Hmm... interesting.  I used the current `master` code with the following
base test case:

    src/emacs -Q --eval '(start-process "toto" "*scratch*" "sh" "-c" "while sleep 1; do echo hi; done")' ~/tmp/foo.c

This triggers the problem where auto-save is done pretty much after
every keystroke (with ~1s delay).  But if I change that to:

    src/emacs -Q --eval '(add-hook `post-command-hook (lambda () (start-process "toto" "*scratch*" "sh" "-c" "sleep 1; echo hi")))' ~/tmp/foo.c

then the problem doesn't seem to occur any more (or rather it still
does, but more rarely, with a pattern I have trouble discerning).
It does occur, OTOH with:

    src/emacs -Q --eval '(add-hook `post-command-hook (lambda () (start-process "toto" "*scratch*" "sh" "-c" "sleep 1; echo hi; sleep 1")))' ~/tmp/foo.c

So it looks like running sentinels does not trigger this problem (it
may even avoid it if the sentinel is run "at the same time" as we
receive output).

With the patch applied, none of the above test cases exhibit the problem
of "A whole lotta auto-saving going".

>> There's one other call to `sit_for` which can be affected:
>>
>> 	  tem0 = sit_for (Vecho_keystrokes, 1, 1);
>> 	  unbind_to (count, Qnil);
>> 	  if (EQ (tem0, Qt)
>> 	      && ! CONSP (Vunread_command_events))
>> 	    echo_now ();
>>
>> I believe it's an improvement there as well.
>
> Why do you believe that?

Because I remember noticing that the echo-keystrokes output sometimes
was displayed earlier than expected (and that it seemed related to
process output).

> It's a different use case, and I don't think we saw any adverse
> effects there from the removal of the buffer-switch "event".
> Are there any adverse effects?

The fact that echo-keystrokes were displayed earlier than expected was
not a serious issue, so I didn't bother to report it.  But I do think
it's an adverse effect.

This said, the current patch has the inverse effect (just like the old
code): auto-save (and echo-keystrokes) can be delayed (potentially
indefinitely) by process output since the `sit_for` call will only
return nil if we waited until timeout without being interrupted by any
process output.  That's why I think in the long run it would be good to
change `sit_for` so it actually does wait the specified time even when
process output comes in (just like The Elisp `sit-for` function does).


        Stefan

Re: A whole lotta auto-saving going

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: larsi@gnus.org,  aaronjensen@gmail.com,  emacs-devel@gnu.org
> Date: Mon, 11 Jan 2021 13:00:33 -0500
> 
>     src/emacs -Q --eval '(start-process "toto" "*scratch*" "sh" "-c" "while sleep 1; do echo hi; done")' ~/tmp/foo.c
> 
> This triggers the problem where auto-save is done pretty much after
> every keystroke (with ~1s delay).  But if I change that to:
> 
>     src/emacs -Q --eval '(add-hook `post-command-hook (lambda () (start-process "toto" "*scratch*" "sh" "-c" "sleep 1; echo hi")))' ~/tmp/foo.c
> 
> then the problem doesn't seem to occur any more (or rather it still
> does, but more rarely, with a pattern I have trouble discerning).
> It does occur, OTOH with:
> 
>     src/emacs -Q --eval '(add-hook `post-command-hook (lambda () (start-process "toto" "*scratch*" "sh" "-c" "sleep 1; echo hi; sleep 1")))' ~/tmp/foo.c

Not sure I understand: what exactly would call post-command-hook in
such a session?  Or do you type something once Emacs starts?

Re: A whole lotta auto-saving going

[Stefan Monnier]

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: larsi@gnus.org,  aaronjensen@gmail.com,  emacs-devel@gnu.org
>> Date: Mon, 11 Jan 2021 13:00:33 -0500
>> 
>>     src/emacs -Q --eval '(start-process "toto" "*scratch*" "sh" "-c"
>> "while sleep 1; do echo hi; done")' ~/tmp/foo.c
>> 
>> This triggers the problem where auto-save is done pretty much after
>> every keystroke (with ~1s delay).  But if I change that to:
>> 
>>     src/emacs -Q --eval '(add-hook `post-command-hook (lambda ()
>> (start-process "toto" "*scratch*" "sh" "-c" "sleep 1; echo hi")))'
>> ~/tmp/foo.c
>> 
>> then the problem doesn't seem to occur any more (or rather it still
>> does, but more rarely, with a pattern I have trouble discerning).
>> It does occur, OTOH with:
>> 
>>     src/emacs -Q --eval '(add-hook `post-command-hook (lambda ()
>> (start-process "toto" "*scratch*" "sh" "-c" "sleep 1; echo hi; sleep
>> 1")))' ~/tmp/foo.c
>
> Not sure I understand: what exactly would call post-command-hook in
> such a session?  Or do you type something once Emacs starts?

Yes, I type random single characters, and the current bug is that the
file is auto-saved pretty much after every character I type (because
soon after you typed the character, the background process emits output,
which interrupts the `sit_for` without there being any pending input
event).


        Stefan

Re: A whole lotta auto-saving going

[T.V Raman via Emacs development discussions.]

Suggest you run M-x shell, build emacs in that buffer, then examine
  buffer-undo-list via describe-variable 
  Aaron Jensen writes:
 > On Sun, Jan 10, 2021 at 12:34 PM T.V Raman <raman@google.com> wrote:
 > >
 > > Tangential (slightly) but given the repro case, perhaps relevant:
 > >
 > > Why is buffer-undo-list   not set up to avoid saving undo information in
 > > shell buffers?
 > 
 > If I can type in a buffer, I'd want to be able to undo, ideally. Is it
 > recording undo steps for things that the shell itself emits? That
 > would seem wrong.
 > 
 > Aaron

-- 
♉Id: kg:/m/0285kf1  🦮♉

--
♉Id: kg:/m/0285kf1  🦮♉

Re: A whole lotta auto-saving going

[Stefan Monnier]

>> It's a different use case, and I don't think we saw any adverse
>> effects there from the removal of the buffer-switch "event".
>> Are there any adverse effects?
>
> The fact that echo-keystrokes were displayed earlier than expected was
> not a serious issue, so I didn't bother to report it.  But I do think
> it's an adverse effect.

Also, in the old code with BUFFER_SWITCH_EVENT there was the same
side-effect, i.e. process output would interrupt the `sit_for` of
echo-keystrokes just like my patch does.

IOW, I don't think my patch is The Right Thing (because that would
involve making `sit_for` loop until the end of the timeout as long as
there's no pending input), but it brings back the old behavior in a "bug
compatible" way.

So unless there's any objection I plan to push the patch below in the
coming days,


        Stefan


diff --git a/src/dispnew.c b/src/dispnew.c
index 36a6dd8a09..e603c67136 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -6049,7 +6049,14 @@ DEFUN ("sleep-for", Fsleep_for, Ssleep_for, 1, 2, 0,
    READING is true if reading input.
    If DISPLAY_OPTION is >0 display process output while waiting.
    If DISPLAY_OPTION is >1 perform an initial redisplay before waiting.
-*/
+
+   Returns a boolean Qt if we waited the full time and returns Qnil if the
+   wait was interrupted by incoming process output or keyboard events.
+
+   FIXME: When `wait_reading_process_output` returns early because of
+   process output, instead of returning nil we should loop and wait some
+   more (i.e. until either there's pending input events or the timeout
+   expired).  */
 
 Lisp_Object
 sit_for (Lisp_Object timeout, bool reading, int display_option)
@@ -6110,8 +6117,9 @@ sit_for (Lisp_Object timeout, bool reading, int display_option)
   gobble_input ();
 #endif
 
-  wait_reading_process_output (sec, nsec, reading ? -1 : 1, do_display,
-			       Qnil, NULL, 0);
+  int nbytes
+    = wait_reading_process_output (sec, nsec, reading ? -1 : 1, do_display,
+			           Qnil, NULL, 0);
 
   if (reading && curbuf_eq_winbuf)
     /* Timers and process filters/sentinels may have changed the selected
@@ -6120,7 +6128,7 @@ sit_for (Lisp_Object timeout, bool reading, int display_option)
        buffer to start with).  */
     set_buffer_internal (XBUFFER (XWINDOW (selected_window)->contents));
 
-  return detect_input_pending () ? Qnil : Qt;
+  return (nbytes > 0 || detect_input_pending ()) ? Qnil : Qt;
 }
 
 

Re: A whole lotta auto-saving going

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> So unless there's any objection I plan to push the patch below in the
> coming days,

Thanks; I see that it's pushed, and I'm running with it now, and it
seems to fix the problem here, too.

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