bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Lawrence Mitchell]

When using word or sexp marking commands, the active region does not
always get highlighted.

emacs -Q
M-<
M-@ # note how ";; This" is selected, and highlighted in region-face
M-@ # ";; This buffer" is selected, however, " buffer" is not highlighted

Pressing C-l at this point, correctly shows the highlighted region.

This is with:

commit e6762a6d2dc65d3201c03d5995686112483dc4ff
Merge: 4ba48d0 6840160
Author: dancol@dancol.org <>
Date:   Sun Feb 3 09:02:56 2013 -0800

    Daniel Colascione 2013-02-03 
    * emacs.c: Use execvp, not execv, when DAEMON_MUST_EXEC

I'd previously updated and built on the 19th of January:

commit c5a8149837c5ed53655d4383dea3b8f29374b266
Author: Glenn Morris <rgm@gnu.org>
Date:   Sat Jan 19 18:40:49 2013 -0800

    * lisp-mode.el (emacs-lisp-mode-map): Add native profiler menu entries.


Which didn't demonstrate this behaviour, if that happens to help with
tracking the issue down.

Cheers,
Lawrence

In GNU Emacs 24.3.50.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.4.2)
 of 2013-02-01 on e4300lm
Windowing system distributor `The X.Org Foundation', version 11.0.11103000
System Description:	Ubuntu 12.04.1 LTS

Configured using:
 `configure --prefix=/home/lmitche4/Apps/emacs -C CFLAGS=-O0 -ggdb
 --no-create --no-recursion'

Important settings:
  value of $LANG: en_GB.UTF-8
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Eli Zaretskii]

> From: Lawrence Mitchell <wence@gmx.li>
> Date: Sun, 03 Feb 2013 22:05:15 +0000
> 
> 
> When using word or sexp marking commands, the active region does not
> always get highlighted.
> 
> emacs -Q
> M-<
> M-@ # note how ";; This" is selected, and highlighted in region-face
> M-@ # ";; This buffer" is selected, however, " buffer" is not highlighted
> 
> Pressing C-l at this point, correctly shows the highlighted region.

Thanks, should be fixed now (revision 111673 on the trunk).

Dmitry, this bug and also 13626 were both caused by your changes in
revision 111647.  While the reason for the changes was to use non-Lisp
objects for some fields, several hunks in the changeset had no
relation whatsoever to that, and were highly questionable.  Example:

  -  /* If showing the region, and mark has changed, we must redisplay
  -     the whole window.  The assignment to this_line_start_pos prevents
  -     the optimization directly below this if-statement.  */
  -  if (((!NILP (Vtransient_mark_mode)
  -       && !NILP (BVAR (XBUFFER (w->buffer), mark_active)))
  -       != !NILP (w->region_showing))
  -      || (!NILP (w->region_showing)
  -         && !EQ (w->region_showing,
  -                 Fmarker_position (BVAR (XBUFFER (w->buffer), mark)))))
  -    CHARPOS (this_line_start_pos) = 0;

I don't understand why such non-trivial code is being dropped on the
floor without discussion.  And there were others like this in this
revision.

Please don't assume that any dropped code that is really needed will
cause bugs that will be immediately reported.  I've seen display bugs
that went unnoticed for months and even years.  In this case, we were
just lucky.

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Lawrence Mitchell]

Eli Zaretskii wrote:
>> From: Lawrence Mitchell <wence@gmx.li>
>> Date: Sun, 03 Feb 2013 22:05:15 +0000
>> Pressing C-l at this point, correctly shows the highlighted region.

> Thanks, should be fixed now (revision 111673 on the trunk).

[...]

Yes, works for me.

Cheers,
Lawrence
-- 
Lawrence Mitchell <wence@gmx.li>

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Eli Zaretskii]

> From: Lawrence Mitchell <wence@gmx.li>
> Date: Mon, 04 Feb 2013 17:20:21 +0000
> 
> Eli Zaretskii wrote:
> >> From: Lawrence Mitchell <wence@gmx.li>
> >> Date: Sun, 03 Feb 2013 22:05:15 +0000
> >> Pressing C-l at this point, correctly shows the highlighted region.
> 
> > Thanks, should be fixed now (revision 111673 on the trunk).
> 
> [...]
> 
> Yes, works for me.

Thanks, closing.

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Dmitry Antipov]

On 02/04/2013 07:49 PM, Eli Zaretskii wrote:

> Dmitry, this bug and also 13626 were both caused by your changes in
> revision 111647.  While the reason for the changes was to use non-Lisp
> objects for some fields, several hunks in the changeset had no
> relation whatsoever to that, and were highly questionable.  Example:
>
>    -  /* If showing the region, and mark has changed, we must redisplay
>    -     the whole window.  The assignment to this_line_start_pos prevents
>    -     the optimization directly below this if-statement.  */
>    -  if (((!NILP (Vtransient_mark_mode)
>    -       && !NILP (BVAR (XBUFFER (w->buffer), mark_active)))
>    -       != !NILP (w->region_showing))
>    -      || (!NILP (w->region_showing)
>    -         && !EQ (w->region_showing,
>    -                 Fmarker_position (BVAR (XBUFFER (w->buffer), mark)))))
>    -    CHARPOS (this_line_start_pos) = 0;

Hm.  Although this is an obvious bug, are you sure that we must redisplay
the whole window even if the region doesn't span multiple lines? IIUC
it should be enough to redisplay the current line only.

Dmitry

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Dmitry Antipov]

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

On 02/05/2013 08:54 AM, Dmitry Antipov wrote:

> Hm.  Although this is an obvious bug, are you sure that we must redisplay
> the whole window even if the region doesn't span multiple lines? IIUC
> it should be enough to redisplay the current line only.

E.g. something like attached. This is the revert of 111673 plus special
treatment of single-line region.

Dmitry


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

=== modified file 'src/window.h'
--- src/window.h	2013-02-04 15:39:55 +0000
+++ src/window.h	2013-02-05 09:58:36 +0000
@@ -333,15 +333,13 @@
        the frame image that window_end_pos did not get onto the frame.  */
     unsigned window_end_valid : 1;
 
+    /* Nonzero if we have highlighted the region (or any part of it).  */
+    unsigned region_showing : 1;
+
     /* Amount by which lines of this window are scrolled in
        y-direction (smooth scrolling).  */
     int vscroll;
 
-    /* If we have highlighted the region (or any part of it), the mark
-       position or -1 (the latter is used by the iterator for internal
-       purposes); otherwise zero.  */
-    ptrdiff_t region_showing;
-
     /* Z_BYTE - buffer position of the last glyph in the current matrix of W.
        Should be nonnegative, and only valid if window_end_valid is nonzero.  */
     ptrdiff_t window_end_bytepos;

=== modified file 'src/xdisp.c'
--- src/xdisp.c	2013-02-04 15:39:55 +0000
+++ src/xdisp.c	2013-02-05 11:59:01 +0000
@@ -2536,8 +2536,8 @@
 
 #endif /* GLYPH_DEBUG and ENABLE_CHECKING */
 
-/* Return mark position if current buffer has the region of non-zero length,
-   or -1 otherwise.  */
+/* Return mark position if current buffer has the region of non-zero
+   length, zero if mark and point are the same, or -1 otherwise.  */
 
 static ptrdiff_t
 markpos_of_region (void)
@@ -2547,9 +2547,7 @@
       && XMARKER (BVAR (current_buffer, mark))->buffer != NULL)
     {
       ptrdiff_t markpos = XMARKER (BVAR (current_buffer, mark))->charpos;
-
-      if (markpos != PT)
-	return markpos;
+      return markpos == PT ? 0 : markpos;
     }
   return -1;
 }
@@ -2689,7 +2687,7 @@
      and IT->region_end_charpos to the start and end of a visible region
      in window IT->w.  Set both to -1 to indicate no region.  */
   markpos = markpos_of_region ();
-  if (0 <= markpos
+  if (0 < markpos
       /* Maybe highlight only in selected window.  */
       && (/* Either show region everywhere.  */
 	  highlight_nonselected_windows
@@ -10753,7 +10751,7 @@
 
   return (((BUF_SAVE_MODIFF (b) < BUF_MODIFF (b)) != w->last_had_star)
 	  || ((!NILP (Vtransient_mark_mode) && !NILP (BVAR (b, mark_active)))
-	      != (w->region_showing != 0)));
+	      != w->region_showing));
 }
 
 /* Nonzero if W has %c in its mode line and mode line should be updated.  */
@@ -12793,7 +12791,7 @@
   int must_finish = 0;
   struct text_pos tlbufpos, tlendpos;
   int number_of_visible_frames;
-  ptrdiff_t count, count1;
+  ptrdiff_t pos, count, count1;
   struct frame *sf;
   int polling_stopped_here = 0;
   Lisp_Object tail, frame;
@@ -12806,6 +12804,9 @@
   /* Non-zero means redisplay has to redisplay the miniwindow.  */
   int update_miniwindow_p = 0;
 
+  /* Non-zero means the mark is on the same line as point.  */
+  bool mark_at_this_line = 0;
+
   TRACE ((stderr, "redisplay_internal %d\n", redisplaying_p));
 
   /* No redisplay if running in batch mode or frame is not yet fully
@@ -13016,23 +13017,17 @@
       clear_garbaged_frames ();
     }
 
-  /* If showing the region, and mark has changed, we must redisplay
-     the whole window.  The assignment to this_line_start_pos prevents
-     the optimization directly below this if-statement.  */
-  if (((!NILP (Vtransient_mark_mode)
-	&& !NILP (BVAR (XBUFFER (w->buffer), mark_active)))
-       != (w->region_showing > 0))
-      || (w->region_showing
-	  && w->region_showing
-	  != XINT (Fmarker_position (BVAR (XBUFFER (w->buffer), mark)))))
-    CHARPOS (this_line_start_pos) = 0;
-
   /* Optimize the case that only the line containing the cursor in the
      selected window has changed.  Variables starting with this_ are
      set in display_line and record information about the line
      containing the cursor.  */
   tlbufpos = this_line_start_pos;
   tlendpos = this_line_end_pos;
+  pos = markpos_of_region ();
+  if (pos != -1)
+    mark_at_this_line = (CHARPOS (tlbufpos) <= pos
+			 && pos <= Z - CHARPOS (tlendpos));
+
   if (!consider_all_windows_p
       && CHARPOS (tlbufpos) > 0
       && !w->update_mode_line
@@ -13048,6 +13043,8 @@
       /* Point must be on the line that we have info recorded about.  */
       && PT >= CHARPOS (tlbufpos)
       && PT <= Z - CHARPOS (tlendpos)
+      /* No region or region which doesn't span multiple lines.  */
+      && (pos == -1 || mark_at_this_line)
       /* All text outside that line, including its final newline,
 	 must be unchanged.  */
       && text_outside_line_unchanged_p (w, CHARPOS (tlbufpos),
@@ -13059,7 +13056,7 @@
 	      || FETCH_BYTE (BYTEPOS (tlbufpos)) == '\n'))
 	/* Former continuation line has disappeared by becoming empty.  */
 	goto cancel;
-      else if (window_outdated (w) || MINI_WINDOW_P (w))
+      else if (window_outdated (w) || MINI_WINDOW_P (w) || mark_at_this_line)
 	{
 	  /* We have to handle the case of continuation around a
 	     wide-column character (see the comment in indent.c around
@@ -13239,8 +13236,6 @@
   ++clear_image_cache_count;
 #endif
 
-  w->region_showing = XINT (Fmarker_position (BVAR (XBUFFER (w->buffer), mark)));
-
   /* Build desired matrices, and update the display.  If
      consider_all_windows_p is non-zero, do it for all windows on all
      frames.  Otherwise do it for selected_window, only.  */
@@ -14837,7 +14832,7 @@
       /* Can't use this case if highlighting a region.  When a
          region exists, cursor movement has to do more than just
          set the cursor.  */
-      && markpos_of_region () < 0
+      && markpos_of_region () <= 0
       && !w->region_showing
       && NILP (Vshow_trailing_whitespace)
       /* This code is not used for mini-buffer for the sake of the case
@@ -15505,7 +15500,7 @@
 
 	  /* If we are highlighting the region, then we just changed
 	     the region, so redisplay to show it.  */
-	  if (0 <= markpos_of_region ())
+	  if (0 < markpos_of_region ())
 	    {
 	      clear_glyph_matrix (w->desired_matrix);
 	      if (!try_window (window, startp, 0))
@@ -16206,7 +16201,7 @@
     return 0;
 
   /* Can't do this if region may have changed.  */
-  if (0 <= markpos_of_region ()
+  if (0 < markpos_of_region ()
       || w->region_showing
       || !NILP (Vshow_trailing_whitespace))
     return 0;
@@ -17038,7 +17033,7 @@
 
   /* Can't use this if highlighting a region because a cursor movement
      will do more than just set the cursor.  */
-  if (0 <= markpos_of_region ())
+  if (0 < markpos_of_region ())
     GIVE_UP (9);
 
   /* Likewise if highlighting trailing whitespace.  */
@@ -19133,7 +19128,7 @@
     }
 
   /* Is IT->w showing the region?  */
-  it->w->region_showing = it->region_beg_charpos > 0 ? -1 : 0;
+  it->w->region_showing = it->region_beg_charpos > 0;
 
   /* Clear the result glyph row and enable it.  */
   prepare_desired_row (row);

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Eli Zaretskii]

> Date: Tue, 05 Feb 2013 16:07:47 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: Lawrence Mitchell <wence@gmx.li>, 13623@debbugs.gnu.org
> 
> > Hm.  Although this is an obvious bug, are you sure that we must redisplay
> > the whole window even if the region doesn't span multiple lines? IIUC
> > it should be enough to redisplay the current line only.
> 
> E.g. something like attached. This is the revert of 111673 plus special
> treatment of single-line region.

If you customize the font of 'region' face, you will see that
additional lines are affected anyway.

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Eli Zaretskii]

> Date: Tue, 05 Feb 2013 08:54:05 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: Lawrence Mitchell <wence@gmx.li>, 13623@debbugs.gnu.org
> 
> >    -  /* If showing the region, and mark has changed, we must redisplay
> >    -     the whole window.  The assignment to this_line_start_pos prevents
> >    -     the optimization directly below this if-statement.  */
> >    -  if (((!NILP (Vtransient_mark_mode)
> >    -       && !NILP (BVAR (XBUFFER (w->buffer), mark_active)))
> >    -       != !NILP (w->region_showing))
> >    -      || (!NILP (w->region_showing)
> >    -         && !EQ (w->region_showing,
> >    -                 Fmarker_position (BVAR (XBUFFER (w->buffer), mark)))))
> >    -    CHARPOS (this_line_start_pos) = 0;
> 
> Hm.  Although this is an obvious bug, are you sure that we must redisplay
> the whole window even if the region doesn't span multiple lines? IIUC
> it should be enough to redisplay the current line only.

As long as we don't restrict the 'region' face to a very small subset
of possible face customizations (e.g., just the background and the
foreground colors), and ignore the other attributes, an arbitrary face
change on one line might potentially affect many more lines in the
window.  E.g., try customizing the 'region' face to twice its normal
height.

And I'm not sure this is worth optimizing anyway: region changes are
relatively rare and almost always driven by user input, so I don't
think redisplay will become significantly faster as result of any
optimizations in this area.

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Dmitry Antipov]

On 02/05/2013 09:45 PM, Eli Zaretskii wrote:

> As long as we don't restrict the 'region' face to a very small subset
> of possible face customizations (e.g., just the background and the
> foreground colors), and ignore the other attributes, an arbitrary face
> change on one line might potentially affect many more lines in the
> window.  E.g., try customizing the 'region' face to twice its normal
> height.

This looks terribly ugly, but works (at least, I don't see any glitches
while performing basic operations with the region).

> And I'm not sure this is worth optimizing anyway: region changes are
> relatively rare and almost always driven by user input, so I don't
> think redisplay will become significantly faster as result of any
> optimizations in this area.

I have a strong suspicion that this also applies to ~50% of xdisp.c
unless you're on 2400bps tty or remote X connection via 33600bps modem.

Dmitry

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Stefan Monnier]

>> And I'm not sure this is worth optimizing anyway: region changes are
>> relatively rare and almost always driven by user input, so I don't
>> think redisplay will become significantly faster as result of any
>> optimizations in this area.
> I have a strong suspicion that this also applies to ~50% of xdisp.c
> unless you're on 2400bps tty or remote X connection via 33600bps modem.

For my use case, the optimisations that matter are the ones that avoid
looking at the unmodified (and mostly all iconified) frames when I work
within a frame or switch between two frames (or maybe 3 at most: the
origin, the destination and the minibuffer-only frame).


        Stefan

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Dmitry Antipov]

On 02/06/2013 06:31 PM, Stefan Monnier wrote:

> For my use case, the optimisations that matter are the ones that avoid
> looking at the unmodified (and mostly all iconified) frames when I work
> within a frame or switch between two frames (or maybe 3 at most: the
> origin, the destination and the minibuffer-only frame).

This is a question of splitting global state between frames because current
tricks like ++windows_or_buffers_changed effectively prevents single-frame
redisplay optimizations.  I have a few experimental patches with per-frame
fonts_changed_p and cursor_type_changed flags. Since font/cursor changes
are rare, the effect is negligible, but this opens the way towards more
interesting things. On the other side, I suspect that the most of users
are either 1) uses single-frame configuration or 2) uses reasonably modern
hardware where the complete redisplay (all frames) is faster than the period
between two keystrokes and so doesn't affect an editing experience.

Dmitry

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Eli Zaretskii]

> Date: Wed, 06 Feb 2013 19:14:43 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: Eli Zaretskii <eliz@gnu.org>, wence@gmx.li, 13623@debbugs.gnu.org
> 
> On the other side, I suspect that the most of users
> are either 1) uses single-frame configuration or 2) uses reasonably modern
> hardware where the complete redisplay (all frames) is faster than the period
> between two keystrokes and so doesn't affect an editing experience.

I doubt many users use only one frame.  I certainly don't, although my
usage patterns are pretty conservative, nowhere near Stefan's.

As for full redisplay: please remember that there are 2 aspects to
that: (1) on the xdisp.c level, which is device independent, and
(2) on the device-dependent xterm.c/w32term.c/nsterm.m etc. level.
Even if on the xdisp.c level we do a complete redisplay of a window,
update_frame and its subroutines compare the desired and the current
display and only redraw the lines that are different.  Therefore, you
could do a complete redisplay on xdisp.c level, and then redraw very
little or even nothing at all, even if your video hardware is 10 years
old.

I'm saying that because due to this 2-level optimization, the hardware
speed is rarely seen in Emacs.

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Eli Zaretskii]

> Date: Wed, 06 Feb 2013 19:14:43 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: Eli Zaretskii <eliz@gnu.org>, wence@gmx.li, 13623@debbugs.gnu.org
> 
> I have a few experimental patches with per-frame fonts_changed_p and
> cursor_type_changed flags. Since font/cursor changes are rare, the
> effect is negligible, but this opens the way towards more
> interesting things.

What exactly is the strategy of enabling per-frame redisplay
optimizations?  As you point out, font and cursor type changes are not
the important cost drivers.  The question is: what is?  IOW, how do
you know which changes affect what frames?

The current display engine does not try to optimize per-frame
redisplay (except on a TTY, but let's forget about that for a moment).
It tries to optimize per-window redisplay.  I don't see what's wrong
with this approach; in fact, I think for GUI frames it's better than
per-frame optimizations.  Therefore, I think we should improve
optimizations by more aggressively detecting windows that need not be
updated, instead of turning our attention to frames.

In any case, the first step towards any intelligent improvements in
redisplay optimizations would be to compile a list of use cases where
we currently give up and redisplay entire windows, and where this
thorough redisplay really matters, i.e. we can actually see and
measure the effect of this.

Another area of redisplay optimizations would be the infamous
very-long-lines use case.  (Personally, I think this one is the single
most important deficiency in the current display engine, by far more
important than any other display problem.)

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Stefan Monnier]

> per-frame optimizations.  Therefore, I think we should improve
> optimizations by more aggressively detecting windows that need not be
> updated, instead of turning our attention to frames.

Agreed.

> In any case, the first step towards any intelligent improvements in
> redisplay optimizations would be to compile a list of use cases where
> we currently give up and redisplay entire windows, and where this

One case where I notice performance sucks is when the minibuffer is in
use (in my minibuffer-only frame), and I switch to another frame and try
to work there.


        Stefan

bug#13623: 24.3.50; Redisplay issue with transient-mark-mode

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Dmitry Antipov <dmantipov@yandex.ru>,  wence@gmx.li,  13623@debbugs.gnu.org
> One case where I notice performance sucks is when the minibuffer is in
> use (in my minibuffer-only frame), and I switch to another frame and try
> to work there.

I suggest to create a new bug report with a recipe to reproduce this.