Why buffer_shared is a mistake

Why buffer_shared is a mistake

[Dmitry Antipov]

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

Look at the screenshot. Here 'A' is an overlay highlighted with font-lock-keyword-face
in buffer *scratch*. What happens if I type C-x C-e (eval-last-sexp) staying  where the
cursor is?

1) Redisplay is invoked with "xaxa" as current_buffer, and buffer_shared is recalculated
    to 2 because "xaxa" is displayed in 2 windows. On leaving redisplay_internal, buffer_shared
    is 2.

2) modify_overlay is called for "*scratch*", and this function still sees buffer_shared as 2
    (unchanged since last redisplay invocation), although *scratch* is displayed in the only
    window.

3) ???

IIUC, as the minimum minorum, buffer_shared should be reset to 0 after leaving redisplay_internal.
Or it should be a per-buffer field.

Dmitry

[-- Attachment #2: redisplay.png --]
[-- Type: image/png, Size: 36204 bytes --]

Re: Why buffer_shared is a mistake

[Eli Zaretskii]

> Date: Thu, 06 Dec 2012 20:02:30 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: Eli Zaretskii <eliz@gnu.org>
> 
> 1) Redisplay is invoked with "xaxa" as current_buffer, and buffer_shared is recalculated
>     to 2 because "xaxa" is displayed in 2 windows. On leaving redisplay_internal, buffer_shared
>     is 2.
> 
> 2) modify_overlay is called for "*scratch*", and this function still sees buffer_shared as 2
>     (unchanged since last redisplay invocation), although *scratch* is displayed in the only
>     window.
> 
> 3) ???
> 
> IIUC, as the minimum minorum, buffer_shared should be reset to 0 after leaving redisplay_internal.
> Or it should be a per-buffer field.

buffer_shared is used only by xdisp.c, with the single exception of
modify_overlay.  How about changing modify_overlay to compute this
value by itself?

IOW, I think modify_overlay shouldn't have used that variable to
begin with.  It's an internal variable used by the display engine.

Re: Why buffer_shared is a mistake

[Dmitry Antipov]

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

On 12/08/2012 05:58 PM, Eli Zaretskii wrote:

> buffer_shared is used only by xdisp.c, with the single exception of
> modify_overlay.  How about changing modify_overlay to compute this
> value by itself?

This is definitely possible (by enhancing window_loop, for example).
But now I'm thinking about maintaining per-buffer window counter,
which value is independent from the redisplay actions.  This opens
the way to a different optimizations (e.g. if we're modifying a buffer
which isn't shown in a window, we can avoid redisplay at all).

(My view of the whole thing is: Emacs global state is huge, and
redisplay is controlled by too much amount of global variables.
This is error-prone and makes some valuable things like threading
too complicated or even impossible. So it's desirable to deduce
redisplay actions from per-buffer/window/frame variables rather
than from the global ones.)

Dmitry


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

=== modified file 'src/buffer.c'
--- src/buffer.c	2012-12-07 07:16:32 +0000
+++ src/buffer.c	2012-12-10 06:14:55 +0000
@@ -547,6 +547,8 @@
   b->base_buffer = NULL;
   /* No one shares the text with us now.  */
   b->indirections = 0;
+  /* No one shows us now.  */
+  b->window_count = 0;
 
   BUF_GAP_SIZE (b) = 20;
   block_input ();
@@ -794,6 +796,8 @@
   b->indirections = -1;
   /* Notify base buffer that we share the text now.  */
   b->base_buffer->indirections++;
+  /* Always -1 for an indirect buffer.  */
+  b->window_count = -1;
 
   b->pt = b->base_buffer->pt;
   b->begv = b->base_buffer->begv;
@@ -1929,10 +1933,16 @@
       eassert (b->indirections == -1);
       b->base_buffer->indirections--;
       eassert (b->base_buffer->indirections >= 0);
+      /* Make sure that we wasn't confused.  */
+      eassert (b->window_count == -1);
     }
   else
-    /* No one shares our buffer text, can free it.  */
-    free_buffer_text (b);
+    {
+      /* No window showing us, right?  */
+      eassert (b->window_count == 0);
+      /* No one shares our buffer text, can free it.  */
+      free_buffer_text (b);
+    }
 
   if (b->newline_cache)
     {
@@ -3880,17 +3890,17 @@
 
   BUF_COMPUTE_UNCHANGED (buf, start, end);
 
-  /* If this is a buffer not in the selected window,
-     we must do other windows.  */
-  if (buf != XBUFFER (XWINDOW (selected_window)->buffer))
-    windows_or_buffers_changed = 1;
-  /* If multiple windows show this buffer, we must do other windows.  */
-  else if (buffer_shared > 1)
-    windows_or_buffers_changed = 1;
-  /* If we modify an overlay at the end of the buffer, we cannot
-     be sure that window end is still valid.  */
-  else if (end >= ZV && start <= ZV)
-    windows_or_buffers_changed = 1;
+  /* If BUF is visible, consider updating the display if ...  */
+  if (buffer_window_count (buf) > 0)
+    {
+      /* ... it's visible in other window than selected,  */
+      if (buf != XBUFFER (XWINDOW (selected_window)->buffer))
+	windows_or_buffers_changed = 1;
+      /* ... or if we modify an overlay at the end of the buffer
+	 and so we cannot be sure that window end is still valid.  */
+      else if (end >= ZV && start <= ZV)
+	windows_or_buffers_changed = 1;
+    }
 
   ++BUF_OVERLAY_MODIFF (buf);
 }
@@ -5125,6 +5135,9 @@
   /* No one will share the text with these buffers, but let's play it safe.  */
   buffer_defaults.indirections = 0;
   buffer_local_symbols.indirections = 0;
+  /* Likewise no one will display them.  */
+  buffer_defaults.window_count = 0;
+  buffer_local_symbols.window_count = 0;
   set_buffer_intervals (&buffer_defaults, NULL);
   set_buffer_intervals (&buffer_local_symbols, NULL);
   /* This is not strictly necessary, but let's make them initialized.  */

=== modified file 'src/buffer.h'
--- src/buffer.h	2012-12-07 07:16:32 +0000
+++ src/buffer.h	2012-12-10 05:20:09 +0000
@@ -775,6 +775,10 @@
      zero means that we're the only owner of this text.  */
   int indirections;
 
+  /* Number of windows showing this buffer. Always -1 for
+     an indirect buffer since it counts as its base buffer.  */
+  int window_count;
+
   /* A non-zero value in slot IDX means that per-buffer variable
      with index IDX has a local value in this buffer.  The index IDX
      for a buffer-local variable is stored in that variable's slot
@@ -1173,7 +1177,15 @@
        + pos + BUF_BEG_ADDR (buf) - BEG_BYTE);
   return STRING_CHAR (p);
 }
-\f
+
+BUFFER_INLINE int
+buffer_window_count (struct buffer *b)
+{
+  if (b->base_buffer)
+    b = b->base_buffer;
+  return b->window_count;
+}
+
 /* Overlays */
 
 /* Return the marker that stands for where OV starts in the buffer.  */

=== modified file 'src/insdel.c'
--- src/insdel.c	2012-12-03 14:13:06 +0000
+++ src/insdel.c	2012-12-10 05:39:05 +0000
@@ -1796,13 +1796,14 @@
 			  ptrdiff_t *preserve_ptr)
 {
   struct buffer *base_buffer;
+  struct buffer *selected = XBUFFER (XWINDOW (selected_window)->buffer);
 
   if (!NILP (BVAR (current_buffer, read_only)))
     Fbarf_if_buffer_read_only ();
 
-  /* Let redisplay consider other windows than selected_window
-     if modifying another buffer.  */
-  if (XBUFFER (XWINDOW (selected_window)->buffer) != current_buffer)
+  /* If modifying another visible buffer, let redisplay
+     consider other windows than selected_window.  */
+  if (selected != current_buffer && buffer_window_count (selected) > 0)
     ++windows_or_buffers_changed;
 
   if (buffer_intervals (current_buffer))

=== modified file 'src/window.c'
--- src/window.c	2012-12-07 08:13:49 +0000
+++ src/window.c	2012-12-10 05:18:10 +0000
@@ -270,6 +270,35 @@
   return w;
 }
 
+/* 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.  */
+
+static void
+adjust_window_count (struct window *w, int arg)
+{
+  eassert (eabs (arg) == 1);
+  if (BUFFERP (w->buffer))
+    {
+      struct buffer *b = XBUFFER (w->buffer);
+      
+      if (b->base_buffer)
+	b = b->base_buffer;
+      b->window_count += arg;
+      eassert (b->window_count >= 0);
+    }
+}
+
+/* Set W's buffer slot to VAL and recompute number
+   of windows showing VAL if it is a buffer.  */
+
+void
+wset_buffer (struct window *w, Lisp_Object val)
+{
+  adjust_window_count (w, -1);
+  w->buffer = val;
+  adjust_window_count (w, 1);
+}
+
 /* Build a frequently used 4-integer (X Y W H) list.  */
 
 static Lisp_Object
@@ -2967,11 +2996,12 @@
 {
   Lisp_Object tail, frame;
 
-  /* A single call to window_loop won't do the job because it only
-     considers frames on the current keyboard.  So loop manually over
-     frames, and handle each one.  */
-  FOR_EACH_FRAME (tail, frame)
-    window_loop (REPLACE_BUFFER_IN_WINDOWS_SAFELY, buffer, 1, frame);
+  if (buffer_window_count (XBUFFER (buffer)) > 0)
+    /* A single call to window_loop won't do the job because it only
+       considers frames on the current keyboard.  So loop manually over
+       frames, and handle each one.  */
+    FOR_EACH_FRAME (tail, frame)
+      window_loop (REPLACE_BUFFER_IN_WINDOWS_SAFELY, buffer, 1, frame);
 }
 \f
 /* If *ROWS or *COLS are too small a size for FRAME, set them to the
@@ -3308,7 +3338,7 @@
 
   if (STRINGP (object))
     object = Fget_buffer (object);
-  if (BUFFERP (object) && BUFFER_LIVE_P (XBUFFER (object)))
+  if (BUFFERP (object) && buffer_window_count (XBUFFER (object)) > 0)
     {
       /* Walk all windows looking for buffer, and force update
 	 of each of those windows.  */
@@ -3391,6 +3421,8 @@
   memcpy ((char *) p + sizeof (struct vectorlike_header),
 	  (char *) o + sizeof (struct vectorlike_header),
 	  word_size * VECSIZE (struct window));
+  /* P's buffer slot was changed.  */
+  adjust_window_count (p, 1);
   XSETWINDOW (parent, p);
 
   p->sequence_number = ++sequence_number;

=== modified file 'src/window.h'
--- src/window.h	2012-11-02 10:34:26 +0000
+++ src/window.h	2012-12-10 05:16:57 +0000
@@ -351,11 +351,6 @@
 /* Most code should use these functions to set Lisp fields in struct
    window.  */
 WINDOW_INLINE void
-wset_buffer (struct window *w, Lisp_Object val)
-{
-  w->buffer = val;
-}
-WINDOW_INLINE void
 wset_frame (struct window *w, Lisp_Object val)
 {
   w->frame = val;
@@ -947,11 +942,6 @@
 
 extern int cursor_type_changed;
 
-/* Number of windows displaying the selected buffer.  Normally this is
-   1, but it can be more.  */
-
-extern int buffer_shared;
-
 /* If *ROWS or *COLS are too small a size for FRAME, set them to the
    minimum allowable size.  */
 
@@ -997,6 +987,8 @@
 extern void temp_output_buffer_show (Lisp_Object);
 extern void replace_buffer_in_windows (Lisp_Object);
 extern void replace_buffer_in_windows_safely (Lisp_Object);
+/* This looks like a setter, but it is a bit special.  */
+extern void wset_buffer (struct window *, Lisp_Object);
 extern void init_window_once (void);
 extern void init_window (void);
 extern void syms_of_window (void);

=== modified file 'src/xdisp.c'
--- src/xdisp.c	2012-12-04 15:15:30 +0000
+++ src/xdisp.c	2012-12-10 07:05:16 +0000
@@ -515,11 +515,6 @@
 
 static int overlay_arrow_seen;
 
-/* Number of windows showing the buffer of the selected
-   window (or another buffer with the same base buffer).  */
-
-int buffer_shared;
-
 /* Vector containing glyphs for an ellipsis `...'.  */
 
 static Lisp_Object default_invis_vector[3];
@@ -10894,9 +10889,8 @@
 static int
 buffer_shared_and_changed (void)
 {
-  /* The variable buffer_shared is set in redisplay_window and
-     indicates that we redisplay a buffer in different windows. */
-  return (buffer_shared > 1 && UNCHANGED_MODIFIED < MODIFF);
+  return (buffer_window_count (current_buffer) > 1
+	  && UNCHANGED_MODIFIED < MODIFF);
 }
 
 /* Nonzero if W doesn't reflect the actual state of current buffer due
@@ -13470,10 +13464,6 @@
       FOR_EACH_FRAME (tail, frame)
 	XFRAME (frame)->updated_p = 0;
 
-      /* Recompute # windows showing selected buffer.  This will be
-	 incremented each time such a window is displayed.  */
-      buffer_shared = 0;
-
       FOR_EACH_FRAME (tail, frame)
 	{
 	  struct frame *f = XFRAME (frame);
@@ -15568,21 +15558,6 @@
   if (mode_line_update_needed (w))
     update_mode_line = 1;
 
-  /* Count number of windows showing the selected buffer.  An indirect
-     buffer counts as its base buffer.  */
-  if (!just_this_one_p)
-    {
-      struct buffer *current_base, *window_base;
-      current_base = current_buffer;
-      window_base = XBUFFER (XWINDOW (selected_window)->buffer);
-      if (current_base->base_buffer)
-	current_base = current_base->base_buffer;
-      if (window_base->base_buffer)
-	window_base = window_base->base_buffer;
-      if (current_base == window_base)
-	buffer_shared++;
-    }
-
   /* Point refers normally to the selected window.  For any other
      window, set up appropriate value.  */
   if (!EQ (window, selected_window))

Re: Why buffer_shared is a mistake

[Eli Zaretskii]

> Date: Mon, 10 Dec 2012 11:29:46 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: emacs-devel@gnu.org
> 
> But now I'm thinking about maintaining per-buffer window counter,
> which value is independent from the redisplay actions.  This opens
> the way to a different optimizations (e.g. if we're modifying a buffer
> which isn't shown in a window, we can avoid redisplay at all).

The idea is fine with me.  Just keep in mind the TTY display, where
windows are not necessarily independent as far as redisplay is
concerned.

> (My view of the whole thing is: Emacs global state is huge, and
> redisplay is controlled by too much amount of global variables.
> This is error-prone and makes some valuable things like threading
> too complicated or even impossible. So it's desirable to deduce
> redisplay actions from per-buffer/window/frame variables rather
> than from the global ones.)

Perhaps a good starting point would be to find all those global
variables that control the display engine, and document, or at least
list them in one place.  Then those of them that become buffer- or
window- or frame-local could be removed from that list.