Re: Reachable killed buffers

[Dmitry Antipov <dmantipov@yandex.ru>]

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

On 09/11/2012 12:15 AM, Stefan Monnier wrote:

> That sounds about right, yes.  Tho, I'd introduce a BUFFER_LIVE_P macro
> to make the code more clear.

It's done in 109976.

Dead frame's buffer_list slot is another place to hold killed buffers for
a while (for live frames, killed buffers are filtered out by store_frame_param),
so the similar removal of killed buffers looks reasonable for dead frames too.

> Also, I'm not 100% sure the specific code you show is safe, because
> modifying the graph of objects during GC is always risky.
>
> I'm fairly confident that the call to window_drain_buffer_lists is safe,
> but for swap_in_global_binding is seems a bit less obvious since it
> might modify objects we may have marked already, but I think it's OK
> as well.

IIUC both BLVs and frame/window buffer lists aren't shared. So, marking
traversal which sees an owner symbol/frame/window for the first time
sees these objects for the first time too (thus mark them correctly).
And even we change buffer list or BLV so the marked object becomes
unreachable, we just create some floating garbage which survives current
GC but will be reclaimed during the next one.

> I don't see any reason to remove the previous comment since the change
> doesn't affect its validity.

IIUC this comment is partially invalid since lisp.h comment says that
Lisp_Buffer_Local_Value can't be forwarded to buffer or kboard object
(and this is really so).

Dmitry


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

=== modified file 'src/alloc.c'
--- src/alloc.c	2012-09-11 04:22:03 +0000
+++ src/alloc.c	2012-09-11 05:17:05 +0000
@@ -5865,6 +5865,32 @@
     mark_buffer (buffer->base_buffer);
 }
 
+/* Remove killed buffers or items whose car is a killed buffer
+   from LIST and return changed LIST.  Called during GC.  */
+
+static inline Lisp_Object
+discard_killed_buffers (Lisp_Object list)
+{
+  Lisp_Object tail, prev, tem;
+
+  for (tail = list, prev = Qnil; CONSP (tail); tail = XCDR (tail))
+    {
+      tem = XCAR (tail);
+      if (CONSP (tem))
+	tem = XCAR (tem);
+      if (BUFFERP (tem) && !BUFFER_LIVE_P (XBUFFER (tem)))
+	{
+	  if (NILP (prev))
+	    list = XCDR (tail);
+	  else
+	    XSETCDR (prev, XCDR (tail));
+	}
+      else
+	prev = tail;
+    }
+  return list;
+}
+
 /* Determine type of generic Lisp_Object and mark it accordingly.  */
 
 void
@@ -6001,20 +6027,41 @@
 	    break;
 
 	  case PVEC_FRAME:
-	    mark_vectorlike (ptr);
-	    mark_face_cache (((struct frame *) ptr)->face_cache);
+	    {
+	      struct frame *f = (struct frame *) ptr;
+
+	      /* For live frames, killed buffers are filtered out by
+		 store_frame_param.  For dead frames, we do it here in
+		 attempt to help GC to reclaim killed buffers faster.  */
+	      if (!FRAME_LIVE_P (f))
+		fset_buffer_list (f, discard_killed_buffers (f->buffer_list));
+
+	      mark_vectorlike (ptr);
+	      mark_face_cache (f->face_cache);
+	    }
 	    break;
 
 	  case PVEC_WINDOW:
 	    {
 	      struct window *w = (struct window *) ptr;
+	      bool leaf = NILP (w->hchild) && NILP (w->vchild);
+	      
+	      /* For live windows, Lisp code filters out killed buffers
+		 from both buffer lists.  For dead windows, we do it here
+		 in attempt to help GC to reclaim killed buffers faster.  */
+	      if (leaf && NILP (w->buffer))
+		{
+		  wset_prev_buffers
+		    (w, discard_killed_buffers (w->prev_buffers));
+		  wset_next_buffers
+		    (w, discard_killed_buffers (w->next_buffers));
+		}
 
 	      mark_vectorlike (ptr);
 	      /* Mark glyphs for leaf windows.  Marking window
 		 matrices is sufficient because frame matrices
 		 use the same glyph memory.  */
-	      if (NILP (w->hchild) && NILP (w->vchild)
-		  && w->current_matrix)
+	      if (leaf && w->current_matrix)
 		{
 		  mark_glyph_matrix (w->current_matrix);
 		  mark_glyph_matrix (w->desired_matrix);
@@ -6081,10 +6128,15 @@
 	  case SYMBOL_LOCALIZED:
 	    {
 	      struct Lisp_Buffer_Local_Value *blv = SYMBOL_BLV (ptr);
-	      /* If the value is forwarded to a buffer or keyboard field,
-		 these are marked when we see the corresponding object.
-		 And if it's forwarded to a C variable, either it's not
-		 a Lisp_Object var, or it's staticpro'd already.  */
+	      Lisp_Object where = blv->where;
+	      /* If the value is set up for a killed buffer or deleted
+		 frame, restore it's global binding.  If the value is
+		 forwarded to a C variable, either it's not a Lisp_Object
+		 var, or it's staticpro'd already.  */
+	      if ((BUFFERP (where) && !BUFFER_LIVE_P (XBUFFER (where)))
+		  || (FRAMEP (where) && !FRAME_LIVE_P (XFRAME (where))))
+		/* FIXME: make sure it's safe during GC.  */ 
+		swap_in_global_binding (ptr);
 	      mark_object (blv->where);
 	      mark_object (blv->valcell);
 	      mark_object (blv->defcell);

=== modified file 'src/window.c'
--- src/window.c	2012-09-11 04:22:03 +0000
+++ src/window.c	2012-09-11 04:26:43 +0000
@@ -176,11 +176,6 @@
   w->new_total = val;
 }
 static inline void
-wset_next_buffers (struct window *w, Lisp_Object val)
-{
-  w->next_buffers = val;
-}
-static inline void
 wset_normal_cols (struct window *w, Lisp_Object val)
 {
   w->normal_cols = val;
@@ -201,11 +196,6 @@
   w->pointm = val;
 }
 static inline void
-wset_prev_buffers (struct window *w, Lisp_Object val)
-{
-  w->prev_buffers = val;
-}
-static inline void
 wset_right_fringe_width (struct window *w, Lisp_Object val)
 {
   w->right_fringe_width = val;

=== modified file 'src/window.h'
--- src/window.h	2012-09-05 17:05:32 +0000
+++ src/window.h	2012-09-11 04:26:43 +0000
@@ -414,7 +414,16 @@
 {
   w->window_end_vpos = val;
 }
-
+WINDOW_INLINE void
+wset_prev_buffers (struct window *w, Lisp_Object val)
+{
+  w->prev_buffers = val;
+}
+WINDOW_INLINE void
+wset_next_buffers (struct window *w, Lisp_Object val)
+{
+  w->next_buffers = val;
+}
 
 /* 1 if W is a minibuffer window.  */