Re: Why buffer_shared is a mistake

[Dmitry Antipov <dmantipov@yandex.ru>]

[-- 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))