Re: [RFC, experimental] save_{excursion,restriction}

[Dmitry Antipov <dmantipov@yandex.ru>]

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

On 07/24/2012 01:37 PM, Stefan Monnier wrote:

> BTW, why use an extra `excursions' field rather than just use a new
> Lisp_Object type for "struct excursion"?
> I think that would be cleaner to either make them into
> Lisp_Pseudovector or Lisp_Misc.

1) "excursion state" is not visible to Lisp programs and so not referenced
via Lisp_Objects, it's just a service thing related to save-excursion
primitive and buffer internals. Just like buffer text for the basic editing
primitives.

2) Lisp_XXX things are designed to be managed by GC, and other use of them
is suspicious and discouraged (embedding Lisp_Markers into struct excursion
is the poor solution in the absence of better alternatives). But, the
main purpose of this design is to avoid any dependency from GC to achieve
the lowest possible overhead of save-excursion.

Dmitry

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

=== modified file 'src/alloc.c'
--- src/alloc.c	2012-07-23 11:15:43 +0000
+++ src/alloc.c	2012-07-24 10:49:56 +0000
@@ -3654,15 +3654,11 @@
   (void)
 {
   register Lisp_Object val;
-  register struct Lisp_Marker *p;
+  register struct Lisp_Marker *m;
 
   val = allocate_misc (Lisp_Misc_Marker);
-  p = XMARKER (val);
-  p->buffer = 0;
-  p->bytepos = 0;
-  p->charpos = 0;
-  p->next = NULL;
-  p->insertion_type = 0;
+  m = XMARKER (val);
+  INIT_MARKER (m, (struct buffer *) NULL, 0, 0, 0);
   return val;
 }
 
@@ -3675,20 +3671,22 @@
   Lisp_Object obj;
   struct Lisp_Marker *m;
 
+  /* Use Fmake_marker to create marker points to nowhere.  */
+  eassert (buf != NULL);
+
   /* No dead buffers here.  */
   eassert (!NILP (BVAR (buf, name)));
 
-  /* Every character is at least one byte.  */
-  eassert (charpos <= bytepos);
+  /* In a single-byte buffer, two positions must be equal.
+     Otherwise, every character is at least one byte.  */
+  if (BUF_Z (buf) == BUF_Z_BYTE (buf))
+    eassert (charpos == bytepos);
+  else
+    eassert (charpos <= bytepos);
 
   obj = allocate_misc (Lisp_Misc_Marker);
   m = XMARKER (obj);
-  m->buffer = buf;
-  m->charpos = charpos;
-  m->bytepos = bytepos;
-  m->insertion_type = 0;
-  m->next = BUF_MARKERS (buf);
-  BUF_MARKERS (buf) = m;
+  INIT_MARKER (m, buf, charpos, bytepos, 0);
   return obj;
 }
 
@@ -5851,6 +5849,8 @@
 static void
 mark_buffer (struct buffer *buffer)
 {
+  struct excursion *ex;
+
   /* This is handled much like other pseudovectors...  */
   mark_vectorlike ((struct Lisp_Vector *) buffer);
 
@@ -5865,6 +5865,15 @@
   mark_overlay (buffer->overlays_before);
   mark_overlay (buffer->overlays_after);
 
+  /* In a struct excursion, markers are allocated in place and
+     invisible for GC, so only the window should be marked.  */
+  for (ex = buffer->excursions; ex; ex = ex->next)
+    {
+      struct Lisp_Vector *w = (struct Lisp_Vector *) ex->window;
+      if (!VECTOR_MARKED_P (w))
+	mark_vectorlike (w);
+    }
+
   /* If this is an indirect buffer, mark its base buffer.  */
   if (buffer->base_buffer && !VECTOR_MARKED_P (buffer->base_buffer))
     mark_buffer (buffer->base_buffer);

=== modified file 'src/buffer.c'
--- src/buffer.c	2012-07-24 06:45:44 +0000
+++ src/buffer.c	2012-07-24 10:01:09 +0000
@@ -366,6 +366,7 @@
   *(BUF_GPT_ADDR (b)) = *(BUF_Z_ADDR (b)) = 0; /* Put an anchor '\0'.  */
   b->text->inhibit_shrinking = 0;
 
+  b->excursions = NULL;
   b->newline_cache = 0;
   b->width_run_cache = 0;
   BVAR (b, width_table) = Qnil;
@@ -578,6 +579,7 @@
   b->begv_byte = b->base_buffer->begv_byte;
   b->zv_byte = b->base_buffer->zv_byte;
 
+  b->excursions = NULL;
   b->newline_cache = 0;
   b->width_run_cache = 0;
   BVAR (b, width_table) = Qnil;

=== modified file 'src/buffer.h'
--- src/buffer.h	2012-07-22 03:44:35 +0000
+++ src/buffer.h	2012-07-24 10:01:50 +0000
@@ -472,6 +472,30 @@
     int inhibit_shrinking;
   };
 
+/* Used to record buffer state for save-excursion.  */
+
+struct excursion
+{
+  /* Saved value of XWINDOW (selected_window).  */
+  struct window *window;
+
+  /* Non-zero if the window above has displayed this buffer.  */
+  unsigned visible : 1;
+
+  /* Non-zero if this buffer has mark active.  */
+  unsigned active : 1;
+
+  /* Saved point.  */
+  struct Lisp_Marker point;
+
+  /* Saved mark.  May point to nowhere.  */
+  struct Lisp_Marker mark;
+
+  /* When the calls to save-excursion are nested, this points
+     to an outer save-excursion state, or NULL otherwise.  */
+  struct excursion *next;
+};
+
 /* Lisp fields in struct buffer are hidden from most code and accessed
    via the BVAR macro, below.  Only select pieces of code, like the GC,
    are allowed to use BUFFER_INTERNAL_FIELD.  */
@@ -856,6 +880,9 @@
   /* Position where the overlay lists are centered.  */
   ptrdiff_t overlay_center;
 
+  /* List of recorded excursions.  */
+  struct excursion *excursions;
+
   /* Changes in the buffer are recorded here for undo, and t means
      don't record anything.  This information belongs to the base
      buffer of an indirect buffer.  But we can't store it in the

=== modified file 'src/editfns.c'
--- src/editfns.c	2012-07-17 07:43:01 +0000
+++ src/editfns.c	2012-07-24 10:59:48 +0000
@@ -223,6 +223,19 @@
   return build_marker (current_buffer, PT, PT_BYTE);
 }
 
+/* Fast path to set point at MARK.  */
+
+static inline void
+set_position (struct Lisp_Marker *mark)
+{
+  if (mark->charpos < BEGV)
+    SET_PT_BOTH (BEGV, BEGV_BYTE);
+  else if (mark->charpos > ZV)
+    SET_PT_BOTH (ZV, ZV_BYTE);
+  else
+    SET_PT_BOTH (mark->charpos, mark->bytepos);
+}
+
 DEFUN ("goto-char", Fgoto_char, Sgoto_char, 1, 1, "NGoto char: ",
        doc: /* Set point to POSITION, a number or marker.
 Beginning of buffer is position (point-min), end is (point-max).
@@ -235,14 +248,7 @@
   if (MARKERP (position)
       && current_buffer == XMARKER (position)->buffer)
     {
-      pos = marker_position (position);
-      if (pos < BEGV)
-	SET_PT_BOTH (BEGV, BEGV_BYTE);
-      else if (pos > ZV)
-	SET_PT_BOTH (ZV, ZV_BYTE);
-      else
-	SET_PT_BOTH (pos, marker_byte_position (position));
-
+      set_position (XMARKER (position));
       return position;
     }
 
@@ -821,104 +827,100 @@
 			      Qnil, Qt, Qnil);
 }
 
-\f
+/* Save current buffer state before entering Fsave_excursion.  */
+
 Lisp_Object
 save_excursion_save (void)
 {
-  int visible = (XBUFFER (XWINDOW (selected_window)->buffer)
-		 == current_buffer);
-
-  return Fcons (Fpoint_marker (),
-		Fcons (Fcopy_marker (BVAR (current_buffer, mark), Qnil),
-		       Fcons (visible ? Qt : Qnil,
-			      Fcons (BVAR (current_buffer, mark_active),
-				     selected_window))));
+  Lisp_Object buf;
+  struct excursion *ex;
+  struct window *w = XWINDOW (selected_window);
+  struct Lisp_Marker *m = XMARKER (BVAR (current_buffer, mark));
+
+  ex = xmalloc (sizeof *ex);
+  ex->window = w;
+  ex->visible = (XBUFFER (w->buffer) == current_buffer);
+  ex->active = !NILP (BVAR (current_buffer, mark_active));
+
+  /* We do not initialize type and gcmarkbit since this marker
+     is never referenced via Lisp_Object and invisible for GC.  */
+  INIT_MARKER (&ex->point, current_buffer, PT, PT_BYTE, 0);
+
+  /* Likewise.  Note that charpos and bytepos may be zero.  */
+  INIT_MARKER (&ex->mark, m->buffer, m->charpos, 
+	       m->bytepos, m->insertion_type);
+
+  ex->next = current_buffer->excursions;
+  current_buffer->excursions = ex;
+  XSETBUFFER (buf, current_buffer);
+  return buf;
 }
 
+/* Restore BUFFER's values before leaving Fsave_excursion.  */
+
 Lisp_Object
-save_excursion_restore (Lisp_Object info)
+save_excursion_restore (Lisp_Object buffer)
 {
-  Lisp_Object tem, tem1, omark, nmark;
-  struct gcpro gcpro1, gcpro2, gcpro3;
-  int visible_p;
-
-  tem = Fmarker_buffer (XCAR (info));
-  /* If buffer being returned to is now deleted, avoid error */
-  /* Otherwise could get error here while unwinding to top level
-     and crash */
-  /* In that case, Fmarker_buffer returns nil now.  */
-  if (NILP (tem))
-    return Qnil;
-
-  omark = nmark = Qnil;
-  GCPRO3 (info, omark, nmark);
-
-  Fset_buffer (tem);
-
-  /* Point marker.  */
-  tem = XCAR (info);
-  Fgoto_char (tem);
-  unchain_marker (XMARKER (tem));
-
-  /* Mark marker.  */
-  info = XCDR (info);
-  tem = XCAR (info);
-  omark = Fmarker_position (BVAR (current_buffer, mark));
-  Fset_marker (BVAR (current_buffer, mark), tem, Fcurrent_buffer ());
-  nmark = Fmarker_position (tem);
-  unchain_marker (XMARKER (tem));
-
-  /* visible */
-  info = XCDR (info);
-  visible_p = !NILP (XCAR (info));
-
-#if 0 /* We used to make the current buffer visible in the selected window
-	 if that was true previously.  That avoids some anomalies.
-	 But it creates others, and it wasn't documented, and it is simpler
-	 and cleaner never to alter the window/buffer connections.  */
-  tem1 = Fcar (tem);
-  if (!NILP (tem1)
-      && current_buffer != XBUFFER (XWINDOW (selected_window)->buffer))
-    Fswitch_to_buffer (Fcurrent_buffer (), Qnil);
-#endif /* 0 */
-
-  /* Mark active */
-  info = XCDR (info);
-  tem = XCAR (info);
-  tem1 = BVAR (current_buffer, mark_active);
-  BVAR (current_buffer, mark_active) = tem;
-
-  /* If mark is active now, and either was not active
-     or was at a different place, run the activate hook.  */
-  if (! NILP (tem))
-    {
-      if (! EQ (omark, nmark))
-        {
-          tem = intern ("activate-mark-hook");
-          Frun_hooks (1, &tem);
-        }
-    }
-  /* If mark has ceased to be active, run deactivate hook.  */
-  else if (! NILP (tem1))
-    {
-      tem = intern ("deactivate-mark-hook");
-      Frun_hooks (1, &tem);
-    }
-
-  /* If buffer was visible in a window, and a different window was
-     selected, and the old selected window is still showing this
-     buffer, restore point in that window.  */
-  tem = XCDR (info);
-  if (visible_p
-      && !EQ (tem, selected_window)
-      && (tem1 = XWINDOW (tem)->buffer,
-	  (/* Window is live...  */
-	   BUFFERP (tem1)
-	   /* ...and it shows the current buffer.  */
-	   && XBUFFER (tem1) == current_buffer)))
-    Fset_window_point (tem, make_number (PT));
-
-  UNGCPRO;
+  struct buffer *buf;
+  struct excursion *ex;
+
+  CHECK_BUFFER (buffer);
+  buf = XBUFFER (buffer);
+  ex = buf->excursions;
+  eassert (ex != NULL);
+
+  /* Restore buffer state only if the buffer is live.
+     Otherwise, just cancel an excursion state.  */
+  if (!NILP (BVAR (buf, name)))
+    {
+      int active;
+      struct Lisp_Marker *m;
+      ptrdiff_t oldmark, newmark;
+
+      /* Restore current buffer.  */
+      set_buffer_internal (buf);
+
+      /* Restore buffer position.  */
+      set_position (&ex->point);
+      unchain_marker (&ex->point);
+
+      /* Restore mark if it was non-zero.  */
+      m = XMARKER (BVAR (buf, mark));
+      oldmark = m->charpos;
+      if (BEGV <= ex->mark.charpos)
+	attach_marker (m, buf, ex->mark.charpos, ex->mark.bytepos);
+      newmark = ex->mark.charpos;
+      unchain_marker (&ex->mark);
+
+      /* If mark and region was active, restore them.  */
+      active = !NILP (BVAR (buf, mark_active));
+      BVAR (buf, mark_active) = ex->active ? Qt : Qnil;
+
+      /* If mark is active now, and either was not active
+	 or was at a different place, run the activate hook.  */
+      if (ex->active && oldmark != newmark)
+	{
+	  Lisp_Object tem = intern ("activate-mark-hook");
+	  Frun_hooks (1, &tem);
+	}
+      /* If mark has ceased to be active, run deactivate hook.  */
+      else if (active)
+	{
+	  Lisp_Object tem = intern ("deactivate-mark-hook");
+	  Frun_hooks (1, &tem);
+	}
+
+      /* If buffer was visible in a window, and a different window
+	 was selected, and the old selected window is still showing
+	 this buffer, restore point in that window.  */
+      if (ex->visible
+	  && ex->window != XWINDOW (selected_window)
+	  && EQ (ex->window->buffer, buffer))
+	set_marker_restricted (ex->window->pointm, make_number (PT), buffer);
+    }
+
+  buf->excursions = ex->next;
+  xfree (ex);
   return Qnil;
 }
 

=== modified file 'src/lisp.h'
--- src/lisp.h	2012-07-24 06:45:44 +0000
+++ src/lisp.h	2012-07-24 10:57:56 +0000
@@ -1283,6 +1283,24 @@
   ptrdiff_t bytepos;
 };
 
+/* Used to setup base fields of just allocated Lisp_Marker.
+   For other markers, consider using attach_marker instead.  */
+
+#define INIT_MARKER(mark, buf, cpos, bpos, itype) \
+  do {						  \
+    (mark)->buffer = (buf);			  \
+    (mark)->charpos = (cpos);			  \
+    (mark)->bytepos = (bpos);			  \
+    (mark)->insertion_type = (itype);		  \
+    if (buf)					  \
+      {						  \
+	(mark)->next = BUF_MARKERS (buf);	  \
+	BUF_MARKERS (buf) = (mark);		  \
+      }						  \
+    else					  \
+      (mark)->next = NULL;			  \
+  } while (0)
+
 /* START and END are markers in the overlay's buffer, and
    PLIST is the overlay's property list.  */
 struct Lisp_Overlay
@@ -2880,8 +2898,10 @@
 extern Lisp_Object set_marker_restricted (Lisp_Object, Lisp_Object, Lisp_Object);
 extern Lisp_Object set_marker_both (Lisp_Object, Lisp_Object, ptrdiff_t, ptrdiff_t);
 extern Lisp_Object set_marker_restricted_both (Lisp_Object, Lisp_Object,
-                                               ptrdiff_t, ptrdiff_t);
+					       ptrdiff_t, ptrdiff_t);
 extern Lisp_Object build_marker (struct buffer *, ptrdiff_t, ptrdiff_t);
+extern void attach_marker (struct Lisp_Marker *, struct buffer *,
+			   ptrdiff_t, ptrdiff_t);
 extern void syms_of_marker (void);
 
 /* Defined in fileio.c */

=== modified file 'src/marker.c'
--- src/marker.c	2012-07-22 05:37:24 +0000
+++ src/marker.c	2012-07-24 10:01:09 +0000
@@ -427,7 +427,7 @@
 
 /* Change M so it points to B at CHARPOS and BYTEPOS.  */
 
-static inline void
+void
 attach_marker (struct Lisp_Marker *m, struct buffer *b,
 	       ptrdiff_t charpos, ptrdiff_t bytepos)
 {