Latest changes in redisplay code

Latest changes in redisplay code

[Eli Zaretskii]

The few last changesets in this area sneaked in questionable changes
which IMO should have been discussed.  I gave one example here:

  http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13623#8

But there are more of them.

  -    /* If this field is nil, it means we don't have a base line.
  -       If it is a buffer, it means don't display the line number
  -       as long as the window shows that buffer.  */
  -    Lisp_Object base_line_pos;
  [...]
  +    /* If this field is zero, it means we don't have a base line.
  +       If it is -1, it means don't display the line number as long
  +       as the window shows its buffer.  */
  +    ptrdiff_t base_line_pos;
  [...]
	  /* If we decided that this buffer isn't suitable for line numbers,
	     don't forget that too fast.  */
  -	if (EQ (w->base_line_pos, w->buffer))
  +	if (w->base_line_pos == -1)
	    goto no_value;
  -	/* But do forget it, if the window shows a different buffer now.  */
  -	else if (BUFFERP (w->base_line_pos))
  -	  wset_base_line_pos (w, Qnil);

This change loses the ability to track the buffer whose line numbers
are being displayed on the mode line.  This means we will now never
show line numbers in this window, even after the buffer it displays
changes.  (I just tried that, and it really is so -- another bug.)

	  /* If the buffer is very big, don't waste time.  */
	  if (INTEGERP (Vline_number_display_limit)
	      && BUF_ZV (b) - BUF_BEGV (b) > XINT (Vline_number_display_limit))
	    {
  -	    wset_base_line_pos (w, Qnil);
  -	    wset_base_line_number (w, Qnil);
  +	    w->base_line_pos = 0;
  +	    w->base_line_number = 0;
	      goto no_value;
	    }

You set base_line_pos here to zero, not to -1: why?

Revision 111584 also has a suspicious change:

  @@ -3189,7 +3182,7 @@ set_window_buffer (Lisp_Object window, L
     wset_window_end_pos (w, make_number (0));
     wset_window_end_vpos (w, make_number (0));
     memset (&w->last_cursor, 0, sizeof w->last_cursor);
  -  wset_window_end_valid (w, Qnil);
  +

Why was this setting dropped?

And what about these two:

  @@ -13758,7 +13756,7 @@ mark_window_display_accurate_1 (struct w
	 else
	  w->last_point = marker_position (w->pointm);

  -      wset_window_end_valid (w, w->buffer);
  +      w->window_end_valid = 1;
  @@ -27782,7 +27779,7 @@ note_mouse_highlight (struct frame *f, i
	And verify the buffer's text has not changed.  */
     b = XBUFFER (w->buffer);
     if (part == ON_TEXT
  -      && EQ (w->window_end_valid, w->buffer)
  +      && w->window_end_valid

Why don't we need to assign a buffer to w->window_end_valid?  What
purpose did this value serve, and why did you decide it was no longer
needed?

I'm afraid we are throwing a lot of babies with bath water here.  This
all is just based on cursory reading of two recent changesets, I
wonder what I might discover if I invest more time.

How can we make this process less dangerous?

Re: Latest changes in redisplay code

[martin rudalics]

> The few last changesets in this area sneaked in questionable changes
> which IMO should have been discussed.

And please, pretty please re-add window sequence numbers.

Thanks, martin

Re: Latest changes in redisplay code

[Dmitry Antipov]

On 02/04/2013 08:25 PM, Eli Zaretskii wrote:

> This change loses the ability to track the buffer whose line numbers
> are being displayed on the mode line.  This means we will now never
> show line numbers in this window, even after the buffer it displays
> changes.  (I just tried that, and it really is so -- another bug.)

Could you please provide an example? I don't see the way to reproduce it.

IIUC the logic for w->base_line_pos before 111647 was:

  nil) initial state, redisplay should calculate it;
  number) calculated by redisplay and so valid, always > 0;
  w-buffer) redisplay has decided that line numbers aren't
  appropriate for w->buffer.

If w->base_line_pos is a number, it maps to:

  0) initial state;
  >0) valid;
  -1) line numbers aren't appropriate.

Note that adjust_window_count tracks all w->buffer changes and invalidates
w->base_line_pos to initial state.

> 	  /* If the buffer is very big, don't waste time.  */
> 	  if (INTEGERP (Vline_number_display_limit)
> 	      && BUF_ZV (b) - BUF_BEGV (b) > XINT (Vline_number_display_limit))
> 	    {
>    -	    wset_base_line_pos (w, Qnil);
>    -	    wset_base_line_number (w, Qnil);
>    +	    w->base_line_pos = 0;
>    +	    w->base_line_number = 0;
> 	      goto no_value;
> 	    }
>
> You set base_line_pos here to zero, not to -1: why?

Because original code sets it to nil.  I suspect that it's needed to reconsider
base_line_pos for the next redisplay if the buffer was changed.

> Revision 111584 also has a suspicious change:
>
>    @@ -3189,7 +3182,7 @@ set_window_buffer (Lisp_Object window, L
>       wset_window_end_pos (w, make_number (0));
>       wset_window_end_vpos (w, make_number (0));
>       memset (&w->last_cursor, 0, sizeof w->last_cursor);
>    -  wset_window_end_valid (w, Qnil);
>    +
>
> Why was this setting dropped?
>
> And what about these two:
>
>    @@ -13758,7 +13756,7 @@ mark_window_display_accurate_1 (struct w
> 	 else
> 	  w->last_point = marker_position (w->pointm);
>
>    -      wset_window_end_valid (w, w->buffer);
>    +      w->window_end_valid = 1;
>    @@ -27782,7 +27779,7 @@ note_mouse_highlight (struct frame *f, i
> 	And verify the buffer's text has not changed.  */
>       b = XBUFFER (w->buffer);
>       if (part == ON_TEXT
>    -      && EQ (w->window_end_valid, w->buffer)
>    +      && w->window_end_valid
>
> Why don't we need to assign a buffer to w->window_end_valid?  What
> purpose did this value serve, and why did you decide it was no longer
> needed?

This is similar to base_line_pos. Before 111584, w->window_end_valid was:

nil) redisplay has not been completed;
w->buffer) redisplay has been completed for _this_ window showing _that_ buffer.
So, if w->buffer is not the same as w->window_end_valid, w->buffer was changed
since last redisplay.

Now it is:

0) redisplay has not been completed
1) redisplay has been completed. We do not need to save the buffer for which
it was completed, it's always w->buffer; any assignment to w->buffer
is tracked by adjust_window_count which invalidates w->window_end_valid.

> How can we make this process less dangerous?

Hm. Although I agree that I had to test 111647 more carefully, there always will
be the things which can't be tested by just one developer (like 111594, for example).
As for the redisplay, automated testing is practically impossible; at least, it would
be nice to have a set of "non-typical" (beyond basic editing) use cases which should
trigger some corner-case situations.

Dmitry

Re: Latest changes in redisplay code

[Eli Zaretskii]

> Date: Tue, 05 Feb 2013 09:52:11 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: Emacs development discussions <emacs-devel@gnu.org>
> 
> On 02/04/2013 08:25 PM, Eli Zaretskii wrote:
> 
> > This change loses the ability to track the buffer whose line numbers
> > are being displayed on the mode line.  This means we will now never
> > show line numbers in this window, even after the buffer it displays
> > changes.  (I just tried that, and it really is so -- another bug.)
> 
> Could you please provide an example? I don't see the way to reproduce it.

Forget it, my testing was wrong.  I always forget that
line-number-display-limit is counted in characters, not lines.  And it
didn't help that the change in adjust_window_count was not mentioned
at all in the ChangeLog entry for this set.  (Please do make sure you
describe all the changes there, it's important for analyzing who
changed what and why.)

In any case, the potential for problems is still there, because now
w->base_line_pos gets reset to zero in many more places than it was
before.  Previously, only xdisp.c would manipulate this attribute in a
few well-defined places.  Now it can be reset by any function that
calls adjust_window_count, including wset_buffer, which is called from
many primitives, most of them outside xdisp.c.  Therefore, it's quite
possible that we will now have to count from the beginning of the
buffer much more frequently than we did before.  In large buffers,
this could be quite a hit.

> > Revision 111584 also has a suspicious change:
> >
> >    @@ -3189,7 +3182,7 @@ set_window_buffer (Lisp_Object window, L
> >       wset_window_end_pos (w, make_number (0));
> >       wset_window_end_vpos (w, make_number (0));
> >       memset (&w->last_cursor, 0, sizeof w->last_cursor);
> >    -  wset_window_end_valid (w, Qnil);
> >    +
> >
> > Why was this setting dropped?

You didn't answer that.

> > How can we make this process less dangerous?
> 
> Hm. Although I agree that I had to test 111647 more carefully, there always will
> be the things which can't be tested by just one developer (like 111594, for example).
> As for the redisplay, automated testing is practically impossible; at least, it would
> be nice to have a set of "non-typical" (beyond basic editing) use cases which should
> trigger some corner-case situations.

Granted, I'm well aware of that.  It's precisely _because_ of this
lack of a comprehensive test suite that I'm worried by such changes.

I can only suggest to try to minimize the danger by thorough
discussions of every non-trivial changeset.  I realize that this will
slow down development to some extent, but I see no other way to make
sure we don't introduce hard to find bugs.

The display code is tricky, and the number of obscure display features
in Emacs is mind-boggling.  The result is that no single individual
can possibly grasp all the possible implications of a change in core
data structures and algorithms.  Discussing such changes can go a long
way towards uncovering potential pitfalls in advance.

Btw, here's one more questionable change, from revision 111583:

  @@ -13784,25 +13777,21 @@ mark_window_display_accurate (Lisp_Objec
     for (; !NILP (window); window = w->next)
       {
	 w = XWINDOW (window);
  -      mark_window_display_accurate_1 (w, accurate_p);
  -
	 if (!NILP (w->vchild))
	  mark_window_display_accurate (w->vchild, accurate_p);
  -      if (!NILP (w->hchild))
  +      else if (!NILP (w->hchild))
	  mark_window_display_accurate (w->hchild, accurate_p);
  +      else if (BUFFERP (w->buffer))
  +       mark_window_display_accurate_1 (w, accurate_p);

Isn't it possible for a window to have both an hchild and a vchild?

Re: Latest changes in redisplay code

[martin rudalics]

 > Isn't it possible for a window to have both an hchild and a vchild?

For heaven's sake, no.

martin

Re: Latest changes in redisplay code

[Dmitry Antipov]

On 02/05/2013 10:01 PM, Eli Zaretskii wrote:

> In any case, the potential for problems is still there, because now
> w->base_line_pos gets reset to zero in many more places than it was
> before.  Previously, only xdisp.c would manipulate this attribute in a
> few well-defined places.  Now it can be reset by any function that
> calls adjust_window_count, including wset_buffer, which is called from
> many primitives, most of them outside xdisp.c.  Therefore, it's quite
> possible that we will now have to count from the beginning of the
> buffer much more frequently than we did before.  In large buffers,
> this could be quite a hit.

Hm. I'll try to reproduce such a situation and check what happens.
Maybe we should invalidate window_end_valid and base_line_pos only
if either window is really switched to another buffer or buffer
is changed.

>>> Revision 111584 also has a suspicious change:
>>>
>>>     @@ -3189,7 +3182,7 @@ set_window_buffer (Lisp_Object window, L
>>>        wset_window_end_pos (w, make_number (0));
>>>        wset_window_end_vpos (w, make_number (0));
>>>        memset (&w->last_cursor, 0, sizeof w->last_cursor);
>>>     -  wset_window_end_valid (w, Qnil);
>>>     +
>>>
>>> Why was this setting dropped?

Because w->window_end_valid is invalidated in wset_buffer, which is
called by set_window_buffer.

> Btw, here's one more questionable change, from revision 111583:
>
>    @@ -13784,25 +13777,21 @@ mark_window_display_accurate (Lisp_Objec
>       for (; !NILP (window); window = w->next)
>         {
> 	 w = XWINDOW (window);
>    -      mark_window_display_accurate_1 (w, accurate_p);
>    -
> 	 if (!NILP (w->vchild))
> 	  mark_window_display_accurate (w->vchild, accurate_p);
>    -      if (!NILP (w->hchild))
>    +      else if (!NILP (w->hchild))
> 	  mark_window_display_accurate (w->hchild, accurate_p);
>    +      else if (BUFFERP (w->buffer))
>    +       mark_window_display_accurate_1 (w, accurate_p);
>
> Isn't it possible for a window to have both an hchild and a vchild?

No. This is documented in window.h and regularly used in window.c,
for example, in delete_all_child_windows.

Dmitry

Re: Latest changes in redisplay code

[Stefan Monnier]

>> Isn't it possible for a window to have both an hchild and a vchild?
> For heaven's sake, no.

Maybe we could clarify it by replacing vchild and hchild by just child
along with a new `orientation' field that can be either `v' or `h'.


        Stefan

Re: Latest changes in redisplay code

[Eli Zaretskii]

> Date: Wed, 06 Feb 2013 11:37:30 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: emacs-devel@gnu.org
> 
> On 02/05/2013 10:01 PM, Eli Zaretskii wrote:
> 
> > In any case, the potential for problems is still there, because now
> > w->base_line_pos gets reset to zero in many more places than it was
> > before.  Previously, only xdisp.c would manipulate this attribute in a
> > few well-defined places.  Now it can be reset by any function that
> > calls adjust_window_count, including wset_buffer, which is called from
> > many primitives, most of them outside xdisp.c.  Therefore, it's quite
> > possible that we will now have to count from the beginning of the
> > buffer much more frequently than we did before.  In large buffers,
> > this could be quite a hit.
> 
> Hm. I'll try to reproduce such a situation and check what happens.
> Maybe we should invalidate window_end_valid and base_line_pos only
> if either window is really switched to another buffer or buffer
> is changed.

That'd be hard, because there are many places that switch to another
buffer temporarily, and they all call adjust_window_count.

Re: Latest changes in redisplay code

[Dmitry Antipov]

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

On 02/06/2013 10:08 PM, Eli Zaretskii wrote:

>> Date: Wed, 06 Feb 2013 11:37:30 +0400
>> From: Dmitry Antipov <dmantipov@yandex.ru>
>> CC: emacs-devel@gnu.org
>>
>> On 02/05/2013 10:01 PM, Eli Zaretskii wrote:
>>
>>> In any case, the potential for problems is still there, because now
>>> w->base_line_pos gets reset to zero in many more places than it was
>>> before.  Previously, only xdisp.c would manipulate this attribute in a
>>> few well-defined places.  Now it can be reset by any function that
>>> calls adjust_window_count, including wset_buffer, which is called from
>>> many primitives, most of them outside xdisp.c.  Therefore, it's quite
>>> possible that we will now have to count from the beginning of the
>>> buffer much more frequently than we did before.  In large buffers,
>>> this could be quite a hit.
>>
>> Hm. I'll try to reproduce such a situation and check what happens.
>> Maybe we should invalidate window_end_valid and base_line_pos only
>> if either window is really switched to another buffer or buffer
>> is changed.
>
> That'd be hard, because there are many places that switch to another
> buffer temporarily, and they all call adjust_window_count.

What about this?

Dmitry



[-- Attachment #2: wset_buffer.patch --]
[-- Type: text/plain, Size: 1850 bytes --]

=== modified file 'src/window.c'
--- src/window.c	2013-02-01 07:23:18 +0000
+++ src/window.c	2013-02-07 05:31:20 +0000
@@ -269,9 +269,10 @@
 }
 
 /* Called when W's buffer slot is changed.  ARG -1 means that W is about to
-   cease its buffer, and 1 means that W is about to set up the new one.  */
+   cease its buffer, and 1 means that W is about to set up the new one.
+   Return W's buffer if it's a buffer or NULL otherwise.  */
 
-static void
+static struct buffer *
 adjust_window_count (struct window *w, int arg)
 {
   eassert (eabs (arg) == 1);
@@ -283,21 +284,34 @@
 	b = b->base_buffer;
       b->window_count += arg;
       eassert (b->window_count >= 0);
-      /* These should be recalculated by redisplay code.  */
-      w->window_end_valid = 0;
-      w->base_line_pos = 0;
+      return b;
     }
+  return NULL;
 }
 
-/* Set W's buffer slot to VAL and recompute number
-   of windows showing VAL if it is a buffer.  */
+/* Set W's buffer slot to VAL and recompute number of windows showing
+   VAL if it is a buffer, catch some redisplay's attention if needed.  */
 
 void
 wset_buffer (struct window *w, Lisp_Object val)
 {
-  adjust_window_count (w, -1);
+  struct buffer *oldb, *newb;
+
+  oldb = adjust_window_count (w, -1);
   w->buffer = val;
-  adjust_window_count (w, 1);
+  newb = adjust_window_count (w, 1);
+  if ((oldb && oldb == newb
+       && ((BUF_UNCHANGED_MODIFIED (oldb) < BUF_MODIFF (oldb))
+	   || (BUF_OVERLAY_UNCHANGED_MODIFIED (oldb)
+	       < BUF_OVERLAY_MODIFF (oldb))))
+      || (!oldb && newb))
+    {
+      /* If W's buffer was changed since last redisplay or W
+	 is switched to another buffer from no buffer, next
+	 redisplay should recalculate these fields.  */
+      w->window_end_valid = 0;
+      w->base_line_pos = 0;
+    }
 }
 
 /* Build a frequently used 4-integer (X Y W H) list.  */

Re: Latest changes in redisplay code

[Eli Zaretskii]

> Date: Thu, 07 Feb 2013 19:42:49 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: emacs-devel@gnu.org
> 
> >> Maybe we should invalidate window_end_valid and base_line_pos only
> >> if either window is really switched to another buffer or buffer
> >> is changed.
> >
> > That'd be hard, because there are many places that switch to another
> > buffer temporarily, and they all call adjust_window_count.
> 
> What about this?

It will still trigger recalculation of lines when we switch to another
buffer temporarily, then back again, won't it?