bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[David Engster]

Recipe:

* emacs -Q

* M-x set-variable RET jit-lock-defer-time RET 0.05 RET

* Call M-x rgrep and search for some string in some directory

You should be able to see that the *grep* buffer capturing grep's output
is not getting fontified until you hit a key. I would expect that it
gets fontified automatically when new output arrives.

Note that the same problem happens for other buffers which capture
process output, like compilation buffers.

-David

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[Eli Zaretskii]

> From: David Engster <deng@randomsample.de>
> Date: Mon, 27 Oct 2014 20:33:48 +0100
> 
> Recipe:
> 
> * emacs -Q
> 
> * M-x set-variable RET jit-lock-defer-time RET 0.05 RET
> 
> * Call M-x rgrep and search for some string in some directory
> 
> You should be able to see that the *grep* buffer capturing grep's output
> is not getting fontified until you hit a key. I would expect that it
> gets fontified automatically when new output arrives.

First, my crystal ball says you omitted something from this recipe,
because if I strictly follow these steps, I cannot even see the
matches produced by Grep, because the shell command inserted by rgrep
into the *grep* buffer is longer than the visible portion of the
window displaying the buffer.  Moreover, rgrep doesn't seem to obey
grep-scroll-output (which will soon be a separate bug report), so you
cannot see the output.

Nevertheless, the problem does exist.  I used the following recipe to
reproduce it:

  emacs -Q
  M-x set-variable RET jit-lock-defer-time RET 0.05 RET
  M-: (setq-default grep-scroll-output t) RET
  M-x grep RET <type a suitable Grep command arguments here> RET

Here's what I saw while digging into this problem:

 . It looks like the idleness state is not reset when we receive input
   from a subprocess.  At least, what I see is that
   jit-lock-deferred-fontify is called only once, prior to any input
   is received from Grep, and never called again, until you press a
   key or send some other input event Emacs's way.

   I can overcome this problem if I add a call to
   internal-timer-start-idle at the end of compilation-handle-exit.
   Not sure this is TRT, though.  If not, what else?

 . But even after the call to internal-timer-start-idle is made when
   Grep exits, and jit-lock-deferred-fontify _is_ called, there's no
   fontification.  Why? because input-pending-p, called by sit-for,
   returns non-nil, and we don't call redisplay.  This happens to me
   on MS-Windows in a GUI session only.  If I put a breakpoint in
   readable_events, here:

  static bool
  readable_events (int flags)
  {
    if (flags & READABLE_EVENTS_DO_TIMERS_NOW)
      timer_check ();

    /* If the buffer contains only FOCUS_IN_EVENT events, and
       READABLE_EVENTS_FILTER_EVENTS is set, report it as empty.  */
    if (kbd_fetch_ptr != kbd_store_ptr)
      {
	if (flags & (READABLE_EVENTS_FILTER_EVENTS
  #ifdef USE_TOOLKIT_SCROLL_BARS
		     | READABLE_EVENTS_IGNORE_SQUEEZABLES
  #endif
		     ))
	  {
	    struct input_event *event;

	    event = ((kbd_fetch_ptr < kbd_buffer + KBD_BUFFER_SIZE)
		     ? kbd_fetch_ptr
		     : kbd_buffer);

	    do
	      {
		if (!(        <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
  #ifdef USE_TOOLKIT_SCROLL_BARS
		      (flags & READABLE_EVENTS_FILTER_EVENTS) &&
  #endif
		      event->kind == FOCUS_IN_EVENT)
  #ifdef USE_TOOLKIT_SCROLL_BARS
		    && !((flags & READABLE_EVENTS_IGNORE_SQUEEZABLES)
			 && (event->kind == SCROLL_BAR_CLICK_EVENT
			     || event->kind == HORIZONTAL_SCROLL_BAR_CLICK_EVENT)
			 && event->part == scroll_bar_handle
			 && event->modifiers == 0)
  #endif
		    )
		  return 1;
		event++;

then I see a single event, BUFFER_SWITCH_EVENT, being reported.
Should input-pending-p ignore such events?

Finally, if I disable blink-cursor-mode, the problem with
input-pending-p doesn't happen, and the only change that is needed to
fix this is a call to internal-timer-start-idle mentioned above.

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[Stefan Monnier]

> You should be able to see that the *grep* buffer capturing grep's output
> is not getting fontified until you hit a key. I would expect that it
> gets fontified automatically when new output arrives.

That's probably because the jit-lock-defer defers fontification to an
idle-timer, but process output is not considered as "activity" so the
idle timers aren't re-run after process output is received.

IOW jit-lock-defer should use a non-idle timer for this case.

Note that an alternative implementation of jit-lock-defer which only
defers when there is not input pending would supposedly not suffer from
this problem since it wouldn't defer fontification in this case (of
course, that would suffer from the reverse problem that by failing to
defer fontification, the redisplay may not be able to keep up with
process output).


        Stefan

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[Stefan Monnier]

> I can overcome this problem if I add a call to
> internal-timer-start-idle at the end of compilation-handle-exit.
> Not sure this is TRT, though.

I don't think this is right: as mentioned the issue is not specific to
compilation, so we'd really need to call internal-timer-start-idle from
the code that runs process-filters.

And it implies a different definition of "idle" than the one we've had
so far, so it'd be an incompatible change.

> then I see a single event, BUFFER_SWITCH_EVENT, being reported.
> Should input-pending-p ignore such events?

I think so, yes, because these events are not (consciously) generated by
the user.


        Stefan

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Tue, 28 Oct 2014 13:09:02 -0400
> Cc: 18856@debbugs.gnu.org
> 
> IOW jit-lock-defer should use a non-idle timer for this case.

But then how do we ensure the fontifications don't happen for as long
as Emacs isn't idle? test idleness by hand inside the timer function?

> Note that an alternative implementation of jit-lock-defer which only
> defers when there is not input pending would supposedly not suffer from
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
You mean, when there _is_ input pending, right?

> this problem since it wouldn't defer fontification in this case (of
> course, that would suffer from the reverse problem that by failing to
> defer fontification, the redisplay may not be able to keep up with
> process output).

Indeed, so what's the point of doing that?

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[Stefan Monnier]

>> IOW jit-lock-defer should use a non-idle timer for this case.
> But then how do we ensure the fontifications don't happen for as long
> as Emacs isn't idle?

Yes, that question did occur to me as well, but I didn't have an
immediate answer, so I decided to stay silent ;-)

> test idleness by hand inside the timer function?

I don't think we can really do that unless we can access the "time since
idleness started" rather than just whether Emacs is idle or not (which
it almost always is when running timer functions).

Another approach would be to cancel that timer from pre-command-hook.

>> Note that an alternative implementation of jit-lock-defer which only
>> defers when there is not input pending would supposedly not suffer from
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> You mean, when there _is_ input pending, right?

Of course, thanks for correcting me.

>> this problem since it wouldn't defer fontification in this case (of
>> course, that would suffer from the reverse problem that by failing to
>> defer fontification, the redisplay may not be able to keep up with
>> process output).
> Indeed, so what's the point of doing that?

To only defer fontification when we know "for sure" that the user is
waiting for further processing.  If jit-lock-defer-time is smaller than
the normal time between key presses, deferring fontification actually
increases the amount of work done by Emacs, since we end up doing
2 redisplays per command (once without fontification, plus another one
with fontification after jit-lock-defer-time passed), so for "normal"
use, it's more efficient not to defer.

BTW, another reason not to defer for process-output is that contrary to
key-commands, process-output is processed more efficiently if we receive
it in large chunks than in small chunks.  So if there's "pending process
output", it's OK to keep redisplaying with fontification, since it just
means that the next time we get to read the process output we'll get more
output, which we'll hence process more efficiently.


        Stefan

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: deng@randomsample.de,  18856@debbugs.gnu.org
> Date: Tue, 28 Oct 2014 15:01:46 -0400
> 
> >> IOW jit-lock-defer should use a non-idle timer for this case.
> > But then how do we ensure the fontifications don't happen for as long
> > as Emacs isn't idle?
> 
> Yes, that question did occur to me as well, but I didn't have an
> immediate answer, so I decided to stay silent ;-)
> 
> > test idleness by hand inside the timer function?
> 
> I don't think we can really do that unless we can access the "time since
> idleness started" rather than just whether Emacs is idle or not (which
> it almost always is when running timer functions).

Doesn't current-idle-time fit the bill?

> >> Note that an alternative implementation of jit-lock-defer which only
> >> defers when there is not input pending would supposedly not suffer from
> >   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > You mean, when there _is_ input pending, right?
> 
> Of course, thanks for correcting me.
> 
> >> this problem since it wouldn't defer fontification in this case (of
> >> course, that would suffer from the reverse problem that by failing to
> >> defer fontification, the redisplay may not be able to keep up with
> >> process output).
> > Indeed, so what's the point of doing that?
> 
> To only defer fontification when we know "for sure" that the user is
> waiting for further processing.

Sorry, you lost me.  When input is pending, what further processing is
the user waiting for?

And anyway, isn't running off an idle timer already an attempt to
defer when there's more input, i.e. Emacs is not idle?

> If jit-lock-defer-time is smaller than the normal time between key
> presses, deferring fontification actually increases the amount of
> work done by Emacs, since we end up doing 2 redisplays per command
> (once without fontification, plus another one with fontification
> after jit-lock-defer-time passed), so for "normal" use, it's more
> efficient not to defer.

I don't think this is too high a price.  First, as Alain established,
what takes 90% of the time is not redisplay, but fontifications, and
those are run only once.  And second, the 2nd redisplay will only
redraw the portions that were fontified, not the entire window.  So
this is not "twice", but more like 1.2 times.

> BTW, another reason not to defer for process-output is that contrary to
> key-commands, process-output is processed more efficiently if we receive
> it in large chunks than in small chunks.  So if there's "pending process
> output", it's OK to keep redisplaying with fontification, since it just
> means that the next time we get to read the process output we'll get more
> output, which we'll hence process more efficiently.

Yes, I actually thought of disabling deferral when a process filter
runs.

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[Stefan Monnier]

>> To only defer fontification when we know "for sure" that the user is
>> waiting for further processing.
> Sorry, you lost me.  When input is pending, what further processing is
> the user waiting for?

The user is waiting for Emacs to process the input that he's already sent.

> I don't think this is too high a price.  First, as Alain established,
> what takes 90% of the time is not redisplay, but fontifications, and
> those are run only once.

This 90% is for CC-mode, where font-lock is particularly expensive.

> And second, the 2nd redisplay will only redraw the portions that were
> fontified, not the entire window.  So this is not "twice", but more
> like 1.2 times.

The 1st redisplay also only redrew the portion that was modified, which
is pretty close in many cases to the portion that later gets fontified.


        Stefan

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: deng@randomsample.de,  18856@debbugs.gnu.org
> Date: Wed, 29 Oct 2014 00:06:14 -0400
> 
> >> To only defer fontification when we know "for sure" that the user is
> >> waiting for further processing.
> > Sorry, you lost me.  When input is pending, what further processing is
> > the user waiting for?
> 
> The user is waiting for Emacs to process the input that he's already sent.

Using an idle timer does that, right?

> > I don't think this is too high a price.  First, as Alain established,
> > what takes 90% of the time is not redisplay, but fontifications, and
> > those are run only once.
> 
> This 90% is for CC-mode, where font-lock is particularly expensive.

Yes, that's right.

> > And second, the 2nd redisplay will only redraw the portions that were
> > fontified, not the entire window.  So this is not "twice", but more
> > like 1.2 times.
> 
> The 1st redisplay also only redrew the portion that was modified, which
> is pretty close in many cases to the portion that later gets fontified.

I was thinking about scrolling with C-v, in which case the first
redisplay redraws almost the entire window.

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: David Engster <deng@randomsample.de>,  18856@debbugs.gnu.org
> Date: Tue, 28 Oct 2014 13:38:36 -0400
> 
> > I can overcome this problem if I add a call to
> > internal-timer-start-idle at the end of compilation-handle-exit.
> > Not sure this is TRT, though.
> 
> I don't think this is right: as mentioned the issue is not specific to
> compilation, so we'd really need to call internal-timer-start-idle from
> the code that runs process-filters.
> 
> And it implies a different definition of "idle" than the one we've had
> so far, so it'd be an incompatible change.
> 
> > then I see a single event, BUFFER_SWITCH_EVENT, being reported.
> > Should input-pending-p ignore such events?
> 
> I think so, yes, because these events are not (consciously) generated by
> the user.

Is the below an OK solution for the issues discussed here?

If OK, I'd like to commit the compile.el part, the one that turns off
jit-lock-deferral in compilation buffers, to the release branch, since
it's a regression from Emacs 23.  OK?

--- src/keyboard.c~0	2014-10-19 07:08:12 +0300
+++ src/keyboard.c	2014-10-29 16:44:02 +0200
@@ -404,6 +404,7 @@ static struct timespec timer_last_idlene
 #define READABLE_EVENTS_DO_TIMERS_NOW		(1 << 0)
 #define READABLE_EVENTS_FILTER_EVENTS		(1 << 1)
 #define READABLE_EVENTS_IGNORE_SQUEEZABLES	(1 << 2)
+#define READABLE_EVENTS_IGNORE_BUFFER_SWITCH	(1 << 3)
 
 /* Function for init_keyboard to call with no args (if nonzero).  */
 static void (*keyboard_init_hook) (void);
@@ -3495,7 +3496,8 @@ readable_events (int flags)
 		       && event->part == scroll_bar_handle
 		       && event->modifiers == 0)
 #endif
-		  )
+		  && !((flags & READABLE_EVENTS_IGNORE_BUFFER_SWITCH)
+		       && event->kind == BUFFER_SWITCH_EVENT))
 		return 1;
 	      event++;
               if (event == kbd_buffer + KBD_BUFFER_SIZE)
@@ -10019,10 +10021,12 @@ If CHECK-TIMERS is non-nil, timers that
   /* Process non-user-visible events (Bug#10195).  */
   process_special_events ();
 
-  return (get_input_pending ((NILP (check_timers)
-                              ? 0 : READABLE_EVENTS_DO_TIMERS_NOW)
-			     | READABLE_EVENTS_FILTER_EVENTS)
-	  ? Qt : Qnil);
+  int flags =
+    READABLE_EVENTS_IGNORE_BUFFER_SWITCH | READABLE_EVENTS_FILTER_EVENTS;
+
+  if (!NILP (check_timers))
+    flags |= READABLE_EVENTS_DO_TIMERS_NOW;
+  return (get_input_pending (flags) ? Qt : Qnil);
 }
 
 DEFUN ("recent-keys", Frecent_keys, Srecent_keys, 0, 0, 0,


--- lisp/progmodes/compile.el~	2014-10-28 15:11:16 +0200
+++ lisp/progmodes/compile.el	2014-10-29 16:50:40 +0200
@@ -1975,6 +1975,12 @@
        compilation-page-delimiter)
   ;; (set (make-local-variable 'compilation-buffer-modtime) nil)
   (compilation-setup)
+  ;; Turn off deferred fontifications in the compilation buffer, if
+  ;; the user turned them on globally.  This is because idle timers
+  ;; aren't re-run after receiving input from a subprocess, so the
+  ;; buffer is left unfontified after the compilation exits, until
+  ;; some other input event happens.
+  (set (make-local-variable 'jit-lock-defer-time) nil)
   (setq buffer-read-only t)
   (run-mode-hooks 'compilation-mode-hook))
 

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[Stefan Monnier]

>> >> To only defer fontification when we know "for sure" that the user is
>> >> waiting for further processing.
>> > Sorry, you lost me.  When input is pending, what further processing is
>> > the user waiting for?
>> The user is waiting for Emacs to process the input that he's already sent.
> Using an idle timer does that, right?

Huh?  No, the user is waiting because Emacs hasn't replied yet.
Idle timers are for when the user hasn't given any command, so Emacs is
waiting, rather than the user.

>> The 1st redisplay also only redrew the portion that was modified, which
>> is pretty close in many cases to the portion that later gets fontified.
> I was thinking about scrolling with C-v, in which case the first
> redisplay redraws almost the entire window.

Same difference: the 1st redisplay redraws almost the whole window, and
the second as well because almost none of the text had been
fontified earlier.

You're right, that the amount of redraw is not always exactly the same,
but I think that in many/most cases the two are comparable.


        Stefan

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[Stefan Monnier]

>  #define READABLE_EVENTS_FILTER_EVENTS		(1 << 1)
>  #define READABLE_EVENTS_IGNORE_SQUEEZABLES	(1 << 2)
> +#define READABLE_EVENTS_IGNORE_BUFFER_SWITCH	(1 << 3)

I think we should not need a new such setting, and can simply use
READABLE_EVENTS_FILTER_EVENTS (i.e. handle BUFFER_SWITCH_EVENT like we
handle FOCUS_IN, and I'm surprised there aren't more in that set of "not
really input" events).


        Stefan

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: deng@randomsample.de,  18856@debbugs.gnu.org
> Date: Wed, 29 Oct 2014 18:16:00 -0400
> 
> >> >> To only defer fontification when we know "for sure" that the user is
> >> >> waiting for further processing.
> >> > Sorry, you lost me.  When input is pending, what further processing is
> >> > the user waiting for?
> >> The user is waiting for Emacs to process the input that he's already sent.
> > Using an idle timer does that, right?
> 
> Huh?  No, the user is waiting because Emacs hasn't replied yet.
> Idle timers are for when the user hasn't given any command, so Emacs is
> waiting, rather than the user.

You said "_defer_ fontification when the user is waiting for Emacs".
When user is waiting for Emacs, idle timers won't run, and therefore
fontifications done in a function that runs off an idle timer will not
be performed.  How does this not fit what you describe?

> >> The 1st redisplay also only redrew the portion that was modified, which
> >> is pretty close in many cases to the portion that later gets fontified.
> > I was thinking about scrolling with C-v, in which case the first
> > redisplay redraws almost the entire window.
> 
> Same difference: the 1st redisplay redraws almost the whole window, and
> the second as well because almost none of the text had been
> fontified earlier.

The amount of redrawing depends on what portions of the visible text
are fontified.

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[Stefan Monnier]

> You said "_defer_ fontification when the user is waiting for Emacs".
> When user is waiting for Emacs, idle timers won't run, and therefore
> fontifications done in a function that runs off an idle timer will not
> be performed.  How does this not fit what you describe?

Your question was:

   Indeed, so what's the point of doing that?

where AFAIK "that" referred to "defer fontification when input is pending".

And I explained that the point of deferring fontification only when
input is pending (as opposed to doing it all the time, as in the
current jit-lock-defer-time system) is that we defer less often, more
specifically we only defer "when we know for sure that the user is
waiting for further processing".

>> Same difference: the 1st redisplay redraws almost the whole window, and
>> the second as well because almost none of the text had been
>> fontified earlier.
> The amount of redrawing depends on what portions of the visible text
> are fontified.

That's why I said "because almost none of the text had been
fontified earlier".  It's not always true, but the end result is the
same: the double redisplay takes extra time.  Does it take exactly twice
the time?  Maybe not exactly, but I think that a factor of 2 is a good
enough approximation.


        Stefan

bug#18856: 24.4; *grep* output buffer not getting fontified when jit-lock-defer-time is used

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: deng@randomsample.de,  18856@debbugs.gnu.org
> Date: Wed, 29 Oct 2014 18:23:50 -0400
> 
> >  #define READABLE_EVENTS_FILTER_EVENTS		(1 << 1)
> >  #define READABLE_EVENTS_IGNORE_SQUEEZABLES	(1 << 2)
> > +#define READABLE_EVENTS_IGNORE_BUFFER_SWITCH	(1 << 3)
> 
> I think we should not need a new such setting, and can simply use
> READABLE_EVENTS_FILTER_EVENTS (i.e. handle BUFFER_SWITCH_EVENT like we
> handle FOCUS_IN, and I'm surprised there aren't more in that set of "not
> really input" events).

I made the requested change in keyboard.c, and committed both parts,
with the compile.el part on the emacs-24 branch.

Closing.