Failing to GC killed buffers considered harmful

Failing to GC killed buffers considered harmful

[Eli Zaretskii]

This recent change on master:

  commit afaf2f465188ab1f438ff3e021260e7c529b1b9d
  Author:     Pip Cet <pipcet@gmail.com>
  AuthorDate: Sat Mar 14 18:26:33 2020 +0000
  Commit:     Eli Zaretskii <eliz@gnu.org>
  CommitDate: Sun Mar 15 16:35:07 2020 +0200

      Make sure we mark reachable killed buffers during GC

      * src/alloc.c (live_buffer_holding): Add ALL_BUFFERS argument for
      returning killed buffers.
      (mark_maybe_object, mark_maybe_pointer): Use the additional
      argument.  (Bug#39962)

causes us to dump killed buffers in some cases.  Presumably, the call
to GC right before we start pdumping fails to collect a killed buffer,
and it ends up being dumped.  When we restore from emacs.pdump, an
Emacs built with --enable-checking on a platform that uses mmap for
buffer text aborts here:

  #ifdef USE_MMAP_FOR_BUFFERS
    if (dumped_with_unexec_p ())
      {
	...
      }
    else
      {
	struct buffer *b;

	/* Only buffers with allocated buffer text should be present at
	   this point in temacs.  */
	FOR_EACH_BUFFER (b)
	  {
	    eassert (b->text->beg != NULL);  <<<<<<<<<<<<<<<<<
	  }
      }
  #endif /* USE_MMAP_FOR_BUFFERS */

because a killed buffer has its text freed and set to NULL.

We could, of course, remove the assertion, but then we are left with a
dead buffer that will never be GC'ed, AFAIU, because objects that come
from the portable dump are considered constantly marked.

Another idea is to avoid dumping such buffers.  But I couldn't find a
facility for doing that safely; simply having dump_buffer return
causes an assertion violation later:

  #ifdef ENABLE_CHECKING
  static Lisp_Object
  dump_check_overlap_dump_reloc (Lisp_Object lreloc_a,
				 Lisp_Object lreloc_b)
  {
    struct dump_reloc reloc_a = dump_decode_dump_reloc (lreloc_a);
    struct dump_reloc reloc_b = dump_decode_dump_reloc (lreloc_b);
    eassert (dump_reloc_get_offset (reloc_a) < dump_reloc_get_offset (reloc_b));
    return Qnil;
  }
  #endif

Daniel, did I miss some facility to avoid dumping an object safely?

If not, what other solutions could we try, given that whether some
pointer into a buffer ends up on the stack at pdump time is something
we cannot really control?

TIA

Re: Failing to GC killed buffers considered harmful

[Stefan Monnier]

> Another idea is to avoid dumping such buffers.  But I couldn't find a
> facility for doing that safely; simply having dump_buffer return
> causes an assertion violation later:

Could we try and write out buffers such that they are read back as
something else (e.g. a special record with type `dead-dumped-buffer`)?


        Stefan

Re: Failing to GC killed buffers considered harmful

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sun, 29 Mar 2020 12:45:48 -0400
> Cc: Daniel Colascione <dancol@dancol.org>, Pip Cet <pipcet@gmail.com>,
>  emacs-devel@gnu.org
> 
> Could we try and write out buffers such that they are read back as
> something else (e.g. a special record with type `dead-dumped-buffer`)?

What problem will that solve?

The other buffers that we inherit from temacs are all frequently-used
ones, so it makes sense to keep them.  A killed buffer isn't useful,
by contrast.

We could perhaps avoid dumping buffers altogether, and instead create
them all at startup, but it will probably also cause complications,
e.g., because the frame we dump must have a window, which must have a
buffer.  I hope that avoiding to dump that one buffer, even if that
requires to add support for this in pdumper.c, will be easier.

Re: Failing to GC killed buffers considered harmful

[Daniel Colascione]

On 3/29/20 11:54 AM, Eli Zaretskii wrote:
>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Date: Sun, 29 Mar 2020 12:45:48 -0400
>> Cc: Daniel Colascione <dancol@dancol.org>, Pip Cet <pipcet@gmail.com>,
>>   emacs-devel@gnu.org
>>
>> Could we try and write out buffers such that they are read back as
>> something else (e.g. a special record with type `dead-dumped-buffer`)?
> 
> What problem will that solve?
> 
> The other buffers that we inherit from temacs are all frequently-used
> ones, so it makes sense to keep them.  A killed buffer isn't useful,
> by contrast.
> 
> We could perhaps avoid dumping buffers altogether, and instead create
> them all at startup, but it will probably also cause complications,
> e.g., because the frame we dump must have a window, which must have a
> buffer.  I hope that avoiding to dump that one buffer, even if that
> requires to add support for this in pdumper.c, will be easier.
> 

None of these measures is necessary. There's nothing special about a 
killed buffer: such a thing is just a lisp buffer object in a specific 
state. We just need to make sure that dumping buffer objects in this 
state works. The right approach isn't to prohibit dumping buffers 
generally for some reason.

And yes, you do have to dump killed buffers. If we have a Lisp reference 
to a buffer and we can't dump buffers, what do we do? Fail the dump? 
Replace the reference with the integer 5?

Yes, if we just remove the assertion, we'll end up with a dead buffer 
that's constantly marked because it lives in the pdumper image. So what? 
All pdumper objects are immortal. (But objects on clean pages do get 
evicted from memory eventually.)

Connecting buffer death to GC at all is just wrong. When we kill a 
buffer, we nuke the buffer's character array and interval tree and put 
the Lisp buffer object in a dead state. Then that object remains live 
(from Lisp POV) until its last reference disappears, at which point we 
reclaim the Lisp object. A buffer should be no different from any other 
pseudovector in this respect.

Re: Failing to GC killed buffers considered harmful

[Eli Zaretskii]

> Cc: pipcet@gmail.com, emacs-devel@gnu.org
> From: Daniel Colascione <dancol@dancol.org>
> Date: Sun, 29 Mar 2020 12:06:56 -0700
> 
> And yes, you do have to dump killed buffers. If we have a Lisp reference 
> to a buffer and we can't dump buffers, what do we do? Fail the dump? 
> Replace the reference with the integer 5?

I don't think we really have a Lisp reference to a killed buffer.  We
just have some random pointer to it in a random place on the stack.

> Yes, if we just remove the assertion, we'll end up with a dead buffer 
> that's constantly marked because it lives in the pdumper image. So what? 

If there's no better way, sure.  That's easy.

Re: Failing to GC killed buffers considered harmful

[Pip Cet]

On Sun, Mar 29, 2020 at 2:22 PM Eli Zaretskii <eliz@gnu.org> wrote:
> This recent change on master:
> causes us to dump killed buffers in some cases.  Presumably, the call
> to GC right before we start pdumping fails to collect a killed buffer,
> and it ends up being dumped.  When we restore from emacs.pdump, an
> Emacs built with --enable-checking on a platform that uses mmap for
> buffer text aborts here:

Not in all cases, I'm afraid.

> We could, of course, remove the assertion, but then we are left with a
> dead buffer that will never be GC'ed, AFAIU, because objects that come
> from the portable dump are considered constantly marked.

The same is true of all other objects, though, right? It seems like
it's simply a bad assert to me.

The problem appears to be that "all_buffers" points to a killed
buffer, and is remembered by pdumper.

Re: Failing to GC killed buffers considered harmful

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 29 Mar 2020 16:46:03 +0000
> Cc: Daniel Colascione <dancol@dancol.org>, emacs-devel@gnu.org
> 
> > We could, of course, remove the assertion, but then we are left with a
> > dead buffer that will never be GC'ed, AFAIU, because objects that come
> > from the portable dump are considered constantly marked.
> 
> The same is true of all other objects, though, right?

You mean, that they are considered marked?  Yes.

> It seems like it's simply a bad assert to me.

Which one?

> The problem appears to be that "all_buffers" points to a killed
> buffer, and is remembered by pdumper.

Yes, because pdumper dumps all the Lisp objects present at that point,
after performing GC.  What else can it do?

Re: Failing to GC killed buffers considered harmful

[Pip Cet]

On Sun, Mar 29, 2020 at 6:48 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sun, 29 Mar 2020 16:46:03 +0000
> > Cc: Daniel Colascione <dancol@dancol.org>, emacs-devel@gnu.org
> >
> > > We could, of course, remove the assertion, but then we are left with a
> > > dead buffer that will never be GC'ed, AFAIU, because objects that come
> > > from the portable dump are considered constantly marked.
> >
> > The same is true of all other objects, though, right?
>
> You mean, that they are considered marked?  Yes.

Well, if my initial experiments are correct, we correctly remove the
killed buffer from the all_buffers list; we simply fail to free the
memory it occupied. That seems like an acceptable loss to me.

> > It seems like it's simply a bad assert to me.
>
> Which one?

The one in buffer.c:

      /* Only buffers with allocated buffer text should be present at
     this point in temacs.  */
      FOR_EACH_BUFFER (b)
        {
      eassert (b->text->beg != NULL);
    }

> > The problem appears to be that "all_buffers" points to a killed
> > buffer, and is remembered by pdumper.
>
> Yes, because pdumper dumps all the Lisp objects present at that point,
> after performing GC.  What else can it do?

My understanding is that pdumper only dumps Lisp objects reachable
from the heap (not the stack). Buffers are an exception only because
of all_buffers, which we could rebuild from the dumped buffers.

The call to garbage_collect in Fdump_emacs_portable doesn't actually
reduce image size.

Re: Failing to GC killed buffers considered harmful

[Daniel Colascione]

On 3/29/20 12:07 PM, Pip Cet wrote:
> On Sun, Mar 29, 2020 at 6:48 PM Eli Zaretskii <eliz@gnu.org> wrote:
>>> From: Pip Cet <pipcet@gmail.com>
>>> Date: Sun, 29 Mar 2020 16:46:03 +0000
>>> Cc: Daniel Colascione <dancol@dancol.org>, emacs-devel@gnu.org
>>>
>>>> We could, of course, remove the assertion, but then we are left with a
>>>> dead buffer that will never be GC'ed, AFAIU, because objects that come
>>>> from the portable dump are considered constantly marked.
>>>
>>> The same is true of all other objects, though, right?
>>
>> You mean, that they are considered marked?  Yes.
> 
> Well, if my initial experiments are correct, we correctly remove the
> killed buffer from the all_buffers list; we simply fail to free the
> memory it occupied. That seems like an acceptable loss to me.
> 
>>> It seems like it's simply a bad assert to me.
>>
>> Which one?
> 
> The one in buffer.c:
> 
>        /* Only buffers with allocated buffer text should be present at
>       this point in temacs.  */
>        FOR_EACH_BUFFER (b)
>          {
>        eassert (b->text->beg != NULL);
>      }
> 
>>> The problem appears to be that "all_buffers" points to a killed
>>> buffer, and is remembered by pdumper.
>>
>> Yes, because pdumper dumps all the Lisp objects present at that point,
>> after performing GC.  What else can it do?
> 
> My understanding is that pdumper only dumps Lisp objects reachable
> from the heap (not the stack). Buffers are an exception only because
> of all_buffers, which we could rebuild from the dumped buffers.
> 
> The call to garbage_collect in Fdump_emacs_portable doesn't actually
> reduce image size.

It runs finalizers, which can free additional memory.

Re: Failing to GC killed buffers considered harmful

[Pip Cet]

On Sun, Mar 29, 2020 at 7:12 PM Daniel Colascione <dancol@dancol.org> wrote:
> On 3/29/20 12:07 PM, Pip Cet wrote:
> > On Sun, Mar 29, 2020 at 6:48 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > The call to garbage_collect in Fdump_emacs_portable doesn't actually
> > reduce image size.
>
> It runs finalizers, which can free additional memory.

Indeed; it can also remove entries from weak hash tables, collect
markers, and collect buffers which would otherwise be dumped. In
practice, though, none of that appears to happen in a standard dump,
at least not to the point where image size is affected. And none of
that can happen predictably with conservative garbage collection.

We could make garbage collection predictable in this one case, by not
marking the stack and making dump-emacs-portable a noreturn function.
I don't think we should.

Re: Failing to GC killed buffers considered harmful

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 29 Mar 2020 19:25:08 +0000
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> 
> We could make garbage collection predictable in this one case, by not
> marking the stack and making dump-emacs-portable a noreturn function.

That'd be a mistake, because our long-term goal is to allow re-dumping
Emacs from a running interactive session.

Re: Failing to GC killed buffers considered harmful

[Pip Cet]

On Sun, Mar 29, 2020 at 7:28 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sun, 29 Mar 2020 19:25:08 +0000
> > Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> >
> > We could make garbage collection predictable in this one case, by not
> > marking the stack and making dump-emacs-portable a noreturn function.
>
> That'd be a mistake, because our long-term goal is to allow re-dumping
> Emacs from a running interactive session.

What would be stored? Is this long-term goal meant to store the stack,
in which case it wouldn't be a very portable dump anymore, or just the
heap, in which case (on GNU) we could simply fork(), call the noreturn
function in the child process, check whether our result file is there
in the parent process, and continue our interactive session from
there...

Re: Failing to GC killed buffers considered harmful

[dancol]

[-- Attachment #1: Type: text/html, Size: 1899 bytes --]

Re: Failing to GC killed buffers considered harmful

[Stefan Monnier]

> That'd be a mistake, because our long-term goal is to allow re-dumping
> Emacs from a running interactive session.

Scanning the stack is definitely not needed for the pdump since the
pdump itself does not include the stack.

So it's only needed if we want the interactive session to *survive*
the pdump.  I can see good reasons to let users create their own
heap-image, but I think the use-cases where it's important for the
session to survive the pdump are pretty hard to come by.


        Stefan

Re: Failing to GC killed buffers considered harmful

[Stefan Monnier]

> The problem appears to be that "all_buffers" points to a killed
> buffer, and is remembered by pdumper.

FWIW, maybe we should get rid of all_buffers.

AFAICT the only reason we need it is to unmark the buffers during the
sweep phase, and we could allocate buffers like any other
pseudovector instead.


        Stefan

Re: Failing to GC killed buffers considered harmful

[Stefan Monnier]

> FWIW, maybe we should get rid of all_buffers.
> AFAICT the only reason we need it is to unmark the buffers during the
> sweep phase, and we could allocate buffers like any other
> pseudovector instead.

The patch below seems to work.

The only worrisome part I think is that `live_buffer_holding` allowed
pointers into buffer objects, whereas `live_vector_p` only treats
pointers to the beginning of the object as a valid reference.

Not sure why buffers would be more likely to have valid pointers into
them (and can't remember discussions about that either), so I assume the
difference was not important (it just happened to be easier to support
that for buffers).

Any objection?


        Stefan


diff --git a/src/alloc.c b/src/alloc.c
index eed73bcdc4..a5e6ad038f 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -432,7 +432,6 @@ no_sanitize_memcpy (void *dest, void const *src, size_t size)
 enum mem_type
 {
   MEM_TYPE_NON_LISP,
-  MEM_TYPE_BUFFER,
   MEM_TYPE_CONS,
   MEM_TYPE_STRING,
   MEM_TYPE_SYMBOL,
@@ -3331,12 +3330,10 @@ allocate_pseudovector (int memlen, int lisplen,
 struct buffer *
 allocate_buffer (void)
 {
-  struct buffer *b = lisp_malloc (sizeof *b, false, MEM_TYPE_BUFFER);
-
+  struct buffer *b
+    = ALLOCATE_PSEUDOVECTOR (struct buffer, cursor_in_non_selected_windows_,
+			     PVEC_BUFFER);
   BUFFER_PVEC_INIT (b);
-  /* Put B on the chain of all buffers including killed ones.  */
-  b->next = all_buffers;
-  all_buffers = b;
   /* Note that the rest fields of B are not initialized.  */
   return b;
 }
@@ -4596,40 +4593,6 @@ live_vector_p (struct mem_node *m, void *p)
   return !NILP (live_vector_holding (m, p));
 }
 
-/* If P is a pointer into a valid buffer object, return the buffer.
-   Otherwise, return nil.  M is a pointer to the mem_block for P.
-   If IGNORE_KILLED is non-zero, treat killed buffers as invalid.  */
-
-static Lisp_Object
-live_buffer_holding (struct mem_node *m, void *p, bool ignore_killed)
-{
-  /* P must point into the block, and the buffer must not
-     have been killed unless ALL-BUFFERS is true.  */
-  if (m->type == MEM_TYPE_BUFFER)
-    {
-      struct buffer *b = m->start;
-      char *cb = m->start;
-      char *cp = p;
-      ptrdiff_t offset = cp - cb;
-      if (0 <= offset && offset < sizeof *b
-	  && !(ignore_killed && NILP (b->name_)))
-	{
-	  Lisp_Object obj;
-	  XSETBUFFER (obj, b);
-	  return obj;
-	}
-    }
-  return Qnil;
-}
-
-/* If P is a pointer into a live (valid and not killed) buffer object,
-   return non-zero.  */
-static bool
-live_buffer_p (struct mem_node *m, void *p)
-{
-  return !NILP (live_buffer_holding (m, p, true));
-}
-
 /* Mark OBJ if we can prove it's a Lisp_Object.  */
 
 static void
@@ -4684,8 +4647,7 @@ mark_maybe_object (Lisp_Object obj)
 	  break;
 
 	case Lisp_Vectorlike:
-	  mark_p = (EQ (obj, live_vector_holding (m, po))
-		    || EQ (obj, live_buffer_holding (m, po, false)));
+	  mark_p = (EQ (obj, live_vector_holding (m, po)));
 	  break;
 
 	default:
@@ -4754,10 +4716,6 @@ mark_maybe_pointer (void *p)
 	  /* Nothing to do; not a pointer to Lisp memory.  */
 	  break;
 
-	case MEM_TYPE_BUFFER:
-	  obj = live_buffer_holding (m, p, false);
-	  break;
-
 	case MEM_TYPE_CONS:
 	  obj = live_cons_holding (m, p);
 	  break;
@@ -5157,9 +5115,6 @@ valid_lisp_object_p (Lisp_Object obj)
     case MEM_TYPE_SPARE:
       return 0;
 
-    case MEM_TYPE_BUFFER:
-      return live_buffer_p (m, p) ? 1 : 2;
-
     case MEM_TYPE_CONS:
       return live_cons_p (m, p);
 
@@ -5976,7 +5931,7 @@ maybe_garbage_collect (void)
 void
 garbage_collect (void)
 {
-  struct buffer *nextb;
+  Lisp_Object tail, buffer;
   char stack_top_variable;
   bool message_p;
   ptrdiff_t count = SPECPDL_INDEX ();
@@ -5992,8 +5947,8 @@ garbage_collect (void)
 
   /* Don't keep undo information around forever.
      Do this early on, so it is no problem if the user quits.  */
-  FOR_EACH_BUFFER (nextb)
-    compact_buffer (nextb);
+  FOR_EACH_LIVE_BUFFER (tail, buffer)
+    compact_buffer (XBUFFER (buffer));
 
   byte_ct tot_before = (profiler_memory_running
 			? total_bytes_of_live_objects ()
@@ -6083,8 +6038,9 @@ garbage_collect (void)
 
   compact_font_caches ();
 
-  FOR_EACH_BUFFER (nextb)
+  FOR_EACH_LIVE_BUFFER (tail, buffer)
     {
+      struct buffer *nextb = XBUFFER (buffer);
       if (!EQ (BVAR (nextb, undo_list), Qt))
 	bset_undo_list (nextb, compact_undo_list (BVAR (nextb, undo_list)));
       /* Now that we have stripped the elements that need not be
@@ -6613,23 +6569,12 @@ #define CHECK_ALLOCATED_AND_LIVE_SYMBOL()	((void) 0)
           = PSEUDOVECTOR_TYPE (ptr);
 
         if (pvectype != PVEC_SUBR &&
-            pvectype != PVEC_BUFFER &&
             !main_thread_p (po))
           CHECK_LIVE (live_vector_p);
 
 	switch (pvectype)
 	  {
 	  case PVEC_BUFFER:
-#if GC_CHECK_MARKED_OBJECTS
-	    {
-	      struct buffer *b;
-	      FOR_EACH_BUFFER (b)
-		if (b == po)
-		  break;
-	      if (b == NULL)
-		emacs_abort ();
-	    }
-#endif /* GC_CHECK_MARKED_OBJECTS */
 	    mark_buffer ((struct buffer *) ptr);
             break;
 
@@ -7108,25 +7053,17 @@ unchain_dead_markers (struct buffer *buffer)
 static void
 sweep_buffers (void)
 {
-  struct buffer *buffer, **bprev = &all_buffers;
+  Lisp_Object tail, buf;
 
   gcstat.total_buffers = 0;
-  for (buffer = all_buffers; buffer; buffer = *bprev)
-    if (!vectorlike_marked_p (&buffer->header))
-      {
-        *bprev = buffer->next;
-        lisp_free (buffer);
-      }
-    else
-      {
-        if (!pdumper_object_p (buffer))
-          XUNMARK_VECTOR (buffer);
-        /* Do not use buffer_(set|get)_intervals here.  */
-        buffer->text->intervals = balance_intervals (buffer->text->intervals);
-        unchain_dead_markers (buffer);
-	gcstat.total_buffers++;
-        bprev = &buffer->next;
-      }
+  FOR_EACH_LIVE_BUFFER (tail, buf)
+    {
+      struct buffer *buffer = XBUFFER (buf);
+      /* Do not use buffer_(set|get)_intervals here.  */
+      buffer->text->intervals = balance_intervals (buffer->text->intervals);
+      unchain_dead_markers (buffer);
+      gcstat.total_buffers++;
+    }
 }
 
 /* Sweep: find all structures not marked, and free them.  */
diff --git a/src/buffer.c b/src/buffer.c
index 4e121ca4ca..355b34a980 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -51,11 +51,6 @@ Copyright (C) 1985-1989, 1993-1995, 1997-2020 Free Software Foundation,
 #include "w32heap.h"		/* for mmap_* */
 #endif
 
-/* First buffer in chain of all buffers (in reverse order of creation).
-   Threaded through ->header.next.buffer.  */
-
-struct buffer *all_buffers;
-
 /* This structure holds the default values of the buffer-local variables
    defined with DEFVAR_PER_BUFFER, that have special slots in each buffer.
    The default value occupies the same slot in this structure
@@ -1786,15 +1781,11 @@ DEFUN ("kill-buffer", Fkill_buffer, Skill_buffer, 0, 1, "bKill buffer: ",
      ask questions or their hooks get errors.  */
   if (!b->base_buffer && b->indirections > 0)
     {
-      struct buffer *other;
+      Lisp_Object tail, other;
 
-      FOR_EACH_BUFFER (other)
-	if (other->base_buffer == b)
-	  {
-	    Lisp_Object buf;
-	    XSETBUFFER (buf, other);
-	    Fkill_buffer (buf);
-	  }
+      FOR_EACH_LIVE_BUFFER (tail, other)
+	if (XBUFFER (other)->base_buffer == b)
+	  Fkill_buffer (other);
 
       /* Exit if we now have killed the base buffer (Bug#11665).  */
       if (!BUFFER_LIVE_P (b))
@@ -2351,10 +2342,10 @@ DEFUN ("buffer-swap-text", Fbuffer_swap_text, Sbuffer_swap_text,
     error ("Cannot swap indirect buffers's text");
 
   { /* This is probably harder to make work.  */
-    struct buffer *other;
-    FOR_EACH_BUFFER (other)
-      if (other->base_buffer == other_buffer
-	  || other->base_buffer == current_buffer)
+    Lisp_Object tail, other;
+    FOR_EACH_LIVE_BUFFER (tail, other)
+      if (XBUFFER (other)->base_buffer == other_buffer
+	  || XBUFFER (other)->base_buffer == current_buffer)
 	error ("One of the buffers to swap has indirect buffers");
   }
 
@@ -2502,7 +2493,7 @@ DEFUN ("set-buffer-multibyte", Fset_buffer_multibyte, Sset_buffer_multibyte,
   (Lisp_Object flag)
 {
   struct Lisp_Marker *tail, *markers;
-  struct buffer *other;
+  Lisp_Object btail, other;
   ptrdiff_t begv, zv;
   bool narrowed = (BEG != BEGV || Z != ZV);
   bool modified_p = !NILP (Fbuffer_modified_p (Qnil));
@@ -2755,13 +2746,16 @@ DEFUN ("set-buffer-multibyte", Fset_buffer_multibyte, Sset_buffer_multibyte,
 
   /* Copy this buffer's new multibyte status
      into all of its indirect buffers.  */
-  FOR_EACH_BUFFER (other)
-    if (other->base_buffer == current_buffer && BUFFER_LIVE_P (other))
-      {
-	BVAR (other, enable_multibyte_characters)
-	  = BVAR (current_buffer, enable_multibyte_characters);
-	other->prevent_redisplay_optimizations_p = 1;
-      }
+  FOR_EACH_LIVE_BUFFER (btail, other)
+    {
+      struct buffer *o = XBUFFER (other);
+      if (o->base_buffer == current_buffer && BUFFER_LIVE_P (o))
+	{
+	  BVAR (o, enable_multibyte_characters)
+	    = BVAR (current_buffer, enable_multibyte_characters);
+	  o->prevent_redisplay_optimizations_p = true;
+	}
+    }
 
   /* Restore the modifiedness of the buffer.  */
   if (!modified_p && !NILP (Fbuffer_modified_p (Qnil)))
@@ -5327,8 +5321,6 @@ init_buffer_once (void)
   Vbuffer_alist = Qnil;
   current_buffer = 0;
   pdumper_remember_lv_ptr_raw (&current_buffer, Lisp_Vectorlike);
-  all_buffers = 0;
-  pdumper_remember_lv_ptr_raw (&all_buffers, Lisp_Vectorlike);
 
   QSFundamental = build_pure_c_string ("Fundamental");
 
@@ -5359,7 +5351,7 @@ init_buffer (void)
 #ifdef USE_MMAP_FOR_BUFFERS
   if (dumped_with_unexec_p ())
     {
-      struct buffer *b;
+      Lisp_Object tail, buffer;
 
 #ifndef WINDOWSNT
       /* These must be reset in the dumped Emacs, to avoid stale
@@ -5381,8 +5373,9 @@ init_buffer (void)
 	 " *code-conversion-work*".  They are created by
 	 init_buffer_once and init_window_once (which are not called
 	 in the dumped Emacs), and by the first call to coding.c routines.  */
-      FOR_EACH_BUFFER (b)
+      FOR_EACH_LIVE_BUFFER (tail, buffer)
         {
+	  struct buffer *b = XBUFFER (buffer);
 	  b->text->beg = NULL;
 	  enlarge_buffer_text (b, 0);
 	}
diff --git a/src/buffer.h b/src/buffer.h
index 31f497ea40..abb1294d03 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -570,9 +570,6 @@ #define BVAR(buf, field) ((buf)->field ## _)
      In an indirect buffer, this is the own_text field of another buffer.  */
   struct buffer_text *text;
 
-  /* Next buffer, in chain of all buffers, including killed ones.  */
-  struct buffer *next;
-
   /* Char position of point in buffer.  */
   ptrdiff_t pt;
 
@@ -1104,15 +1101,6 @@ BUFFER_CHECK_INDIRECTION (struct buffer *b)
     }
 }
 
-/* Chain of all buffers, including killed ones.  */
-
-extern struct buffer *all_buffers;
-
-/* Used to iterate over the chain above.  */
-
-#define FOR_EACH_BUFFER(b) \
-  for ((b) = all_buffers; (b); (b) = (b)->next)
-
 /* This structure holds the default values of the buffer-local variables
    that have special slots in each buffer.
    The default value occupies the same slot in this structure
diff --git a/src/pdumper.c b/src/pdumper.c
index e52163cdae..0f3835578a 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -2845,8 +2844,6 @@ dump_buffer (struct dump_context *ctx, const struct buffer *in_buffer)
      ctx->obj_offset + dump_offsetof (struct buffer, text),
      base_offset + dump_offsetof (struct buffer, own_text));
 
-  dump_field_lv_rawptr (ctx, out, buffer, &buffer->next,
-                        Lisp_Vectorlike, WEIGHT_NORMAL);
   DUMP_FIELD_COPY (out, buffer, pt);
   DUMP_FIELD_COPY (out, buffer, pt_byte);
   DUMP_FIELD_COPY (out, buffer, begv);

Re: Failing to GC killed buffers considered harmful

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Daniel Colascione <dancol@dancol.org>,
>   emacs-devel@gnu.org
> Date: Mon, 30 Mar 2020 14:32:30 -0400
> 
> > FWIW, maybe we should get rid of all_buffers.
> > AFAICT the only reason we need it is to unmark the buffers during the
> > sweep phase, and we could allocate buffers like any other
> > pseudovector instead.
> 
> The patch below seems to work.
> 
> The only worrisome part I think is that `live_buffer_holding` allowed
> pointers into buffer objects, whereas `live_vector_p` only treats
> pointers to the beginning of the object as a valid reference.
> 
> Not sure why buffers would be more likely to have valid pointers into
> them (and can't remember discussions about that either), so I assume the
> difference was not important (it just happened to be easier to support
> that for buffers).
> 
> Any objection?

What are the advantages?  The original problem is solved, and
everybody agreed that having a dead buffer in the pdumped area is
nothing we should bother about.

Re: Failing to GC killed buffers considered harmful

[Daniel Colascione]

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  Daniel Colascione
>> <dancol@dancol.org>,
>>   emacs-devel@gnu.org
>> Date: Mon, 30 Mar 2020 14:32:30 -0400
>>
>> > FWIW, maybe we should get rid of all_buffers.
>> > AFAICT the only reason we need it is to unmark the buffers during the
>> > sweep phase, and we could allocate buffers like any other
>> > pseudovector instead.
>>
>> The patch below seems to work.
>>
>> The only worrisome part I think is that `live_buffer_holding` allowed
>> pointers into buffer objects, whereas `live_vector_p` only treats
>> pointers to the beginning of the object as a valid reference.
>>
>> Not sure why buffers would be more likely to have valid pointers into
>> them (and can't remember discussions about that either), so I assume the
>> difference was not important (it just happened to be easier to support
>> that for buffers).
>>
>> Any objection?
>
> What are the advantages?  The original problem is solved, and
> everybody agreed that having a dead buffer in the pdumped area is
> nothing we should bother about.
>

With Stefan's patch, Emacs is simpler. Simplicity is its own reward.

Re: Failing to GC killed buffers considered harmful

[Stefan Monnier]

>> > FWIW, maybe we should get rid of all_buffers.
>> > AFAICT the only reason we need it is to unmark the buffers during the
>> > sweep phase, and we could allocate buffers like any other
>> > pseudovector instead.
>> 
>> The patch below seems to work.
>> 
>> The only worrisome part I think is that `live_buffer_holding` allowed
>> pointers into buffer objects, whereas `live_vector_p` only treats
>> pointers to the beginning of the object as a valid reference.
>> 
>> Not sure why buffers would be more likely to have valid pointers into
>> them (and can't remember discussions about that either), so I assume the
>> difference was not important (it just happened to be easier to support
>> that for buffers).
>> 
>> Any objection?
>
> What are the advantages?

Simpler code.

> The original problem is solved, and everybody agreed that having
> a dead buffer in the pdumped area is nothing we should bother about.

Indeed, my patch doesn't have anything to do with the original problem,
except that it made me realize this part of the code was not needed
any more.


        Stefan

Re: Failing to GC killed buffers considered harmful

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: pipcet@gmail.com,  dancol@dancol.org,  emacs-devel@gnu.org
> Date: Mon, 30 Mar 2020 15:40:31 -0400
> 
> > What are the advantages?
> 
> Simpler code.

<Shrug> I'm not sure I see the simplification, certainly not a
significant one that would justify the risks.  You've replaced one
macro with a similar (and subtly different) one which is used in the
same way.  You've also removed a single variable with a very simple
structure and use pattern.  I'm not sure I see the gains.

On the downside, I see a few possible pitfalls:

 . How did you test this?  In particularly, did it survive the test in
   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39962#116?

 . We will be tracking buffers via Vbuffer_alist, from which we remove
   the killed buffer in kill-buffer much earlier than from
   all_buffers, and not inside a critical section -- cannot this cause
   some problems if there's a race in updating Vbuffer_alist?

 . What about the following comment earlier in this thread:

   > And yes, you do have to dump killed buffers. If we have a Lisp reference 
   > to a buffer and we can't dump buffers, what do we do? Fail the dump? 
   > Replace the reference with the integer 5?

   Does this issue no longer bother us?  If so, what changed our
   minds?

So I'm not sure why would we want to take the risk of breaking things
due to this change, given that the gains are minor at best.  Will this
allow us to add some useful feature or make some other worthy change
that is impossible or impractical/hard without the change?

Re: Failing to GC killed buffers considered harmful

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 30 Mar 2020 14:32:30 -0400
> Cc: Eli Zaretskii <eliz@gnu.org>, Daniel Colascione <dancol@dancol.org>,
>  emacs-devel@gnu.org
> 
> > FWIW, maybe we should get rid of all_buffers.
> > AFAICT the only reason we need it is to unmark the buffers during the
> > sweep phase, and we could allocate buffers like any other
> > pseudovector instead.
> 
> The patch below seems to work.
> 
> The only worrisome part I think is that `live_buffer_holding` allowed
> pointers into buffer objects, whereas `live_vector_p` only treats
> pointers to the beginning of the object as a valid reference.
> 
> Not sure why buffers would be more likely to have valid pointers into
> them (and can't remember discussions about that either)

Pointers to buffer text come to mind.

> -        if (!pdumper_object_p (buffer))
> -          XUNMARK_VECTOR (buffer);

Why is this part being removed?

Re: Failing to GC killed buffers considered harmful

[Stefan Monnier]

>> Not sure why buffers would be more likely to have valid pointers into
>> them (and can't remember discussions about that either)
>
> Pointers to buffer text come to mind.

In the mean time Pip pointed out that I was confused: pointrs into the
middle of pseudovectors are handled just as well, so it's a non-issue.

>> -        if (!pdumper_object_p (buffer))
>> -          XUNMARK_VECTOR (buffer);
> Why is this part being removed?

Because this is handled by the sweep_vectors code.


        Stefan

Re: Failing to GC killed buffers considered harmful

[Pip Cet]

On Mon, Mar 30, 2020 at 6:32 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> > FWIW, maybe we should get rid of all_buffers.
> > AFAICT the only reason we need it is to unmark the buffers during the
> > sweep phase, and we could allocate buffers like any other
> > pseudovector instead.

I agree that removing all_buffers would be good.

> The patch below seems to work.

But it changes gcstat definitions (killed buffers are no longer
counted towards total_buffers).

> The only worrisome part I think is that `live_buffer_holding` allowed
> pointers into buffer objects, whereas `live_vector_p` only treats
> pointers to the beginning of the object as a valid reference.

Uh, I don't see how the second part of that statement is true. Can you explain?

> Not sure why buffers would be more likely to have valid pointers into
> them (and can't remember discussions about that either),

buffer text.

> so I assume the
> difference was not important (it just happened to be easier to support
> that for buffers).

Which difference, I still don't understand.

Next, it might be nice to have anonymous buffers that are
garbage-collected automatically and don't need to be killed. *ducks*

Re: Failing to GC killed buffers considered harmful

[Stefan Monnier]

>> The patch below seems to work.
> But it changes gcstat definitions (killed buffers are no longer
> counted towards total_buffers).

Indeed (instead all the `struct buffer` objects are now included in the
count of (pseudo)vectors).  I find this of no consequence and I don't
think the GC algorithm should adapt to the gcstat (it should be the
other way around).

>> The only worrisome part I think is that `live_buffer_holding` allowed
>> pointers into buffer objects, whereas `live_vector_p` only treats
>> pointers to the beginning of the object as a valid reference.
> Uh, I don't see how the second part of that statement is true.

Indeed it's not.  I had forgotten that Paul had also changed the
live_vector_holding code in the same way.  Great!

>> Not sure why buffers would be more likely to have valid pointers into
>> them (and can't remember discussions about that either),
> buffer text.

The buffer text is not stored within the `struct buffer` object, so no.
But that's irrelevant since I was confused anyway.

>> so I assume the difference was not important (it just happened to be
>> easier to support that for buffers).
> Which difference, I still don't understand.

You do ;-)

> Next, it might be nice to have anonymous buffers that are
> garbage-collected automatically and don't need to be killed. *ducks*

Did I hear something or was it just the wind?


        Stefan

Re: Failing to GC killed buffers considered harmful

[Pip Cet]

On Tue, Mar 31, 2020 at 4:52 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> >> The patch below seems to work.
> > But it changes gcstat definitions (killed buffers are no longer
> > counted towards total_buffers).
>
> Indeed (instead all the `struct buffer` objects are now included in the
> count of (pseudo)vectors).
> I find this of no consequence and I don't
> think the GC algorithm should adapt to the gcstat (it should be the
> other way around).

Hmm. My argument is circular, and thus invalid: with all_buffers,
having many killed but uncollectable buffers is a problem, so we
should include accurate information about that in report-emacs-bug.
Without all_buffers, killed buffers only eat a bit of memory, so
there's no negative consequence to not counting them accurately.

> > buffer text.
> The buffer text is not stored within the `struct buffer` object, so no.

struct buffer_text, I meant. If we ever think about collecting the
actual text of a buffer because it's being garbage-collected, that
would be something different.

> > Next, it might be nice to have anonymous buffers that are
> > garbage-collected automatically and don't need to be killed. *ducks*
> Did I hear something or was it just the wind?

Well, I have created temporary buffers to act as, essentially, mutable
strings. I see no reason I have to kill
expensively-but-efficiently-implemented mutable strings explicitly
before having them collected! More seriously, I do think making
buffers more lightweight would be a good thing, perhaps even having an
immutable string without text properties/mutable string with gap and
some buffer properties/full-blown buffer spectrum.

(As I realize someone here usually demands use cases: I'd like to fix
the hash table printing code to print hashes predictably. That
requires sorting their keys by something predictable. Their printed
presentation is usually good enough, but that requires
prin1-to-string, which uses a buffer. It's currently too slow.)

> Indeed, my patch doesn't have anything to do with the original problem,

I don't see how the original problem would fail to be solved by your
patch. Pdumper doesn't dump all objects that survive GC, only those it
can reach from the heap (plus some collateral damage). So your patch
restores that requirement precisely, and we can keep the assert()
unless we decide we want to relax our standards and allow killed
buffers during the dump.

Re: Failing to GC killed buffers considered harmful

[Stefan Monnier]

>> > buffer text.
>> The buffer text is not stored within the `struct buffer` object, so no.
> struct buffer_text, I meant.

Ah, right.  We do have pointers into that substructure coming from
indirect buffers.  These should all be accompanied by a `base_buffer`
pointer to the head of the `struct buffer`, so it's not 100% crucial
that we take those inner pointers into account, but yes, it does mean
that inner pointers into `struct buffer` are probably more likely than
for other pseudovectors.

Luckily, Paul's code already takes care of inner pointers into
pseudovectors, so we're good in either case.

> Well, I have created temporary buffers to act as, essentially, mutable
> strings. I see no reason I have to kill
> expensively-but-efficiently-implemented mutable strings explicitly
> before having them collected! More seriously, I do think making
> buffers more lightweight would be a good thing, perhaps even having an
> immutable string without text properties/mutable string with gap and
> some buffer properties/full-blown buffer spectrum.

You don't have to convince me.  I just haven't tried to solve this
long-standing need, and it's a can of worms, I think.

> (As I realize someone here usually demands use cases: I'd like to fix
> the hash table printing code to print hashes predictably.  That
> requires sorting their keys by something predictable. Their printed
> presentation is usually good enough, but that requires
> prin1-to-string, which uses a buffer. It's currently too slow.)

Have you tried to profile it to see where the time is spent, to have
a vague idea of what kind of slimming regime we'd need to impose to our
buffers before it becomes tolerable?

In any case, printing-then-sorting doesn't sound like a good plan:
I think it's inherently inefficient.

>> Indeed, my patch doesn't have anything to do with the original problem,
> I don't see how the original problem would fail to be solved by your patch.

Oh, maybe it would, but that was definitely not my motivation.

> Pdumper doesn't dump all objects that survive GC, only those it
> can reach from the heap (plus some collateral damage). So your patch
> restores that requirement precisely, and we can keep the assert()
> unless we decide we want to relax our standards and allow killed
> buffers during the dump.

I think if we want to re-introduce this assert, we should add a matching
test during pdump.


        Stefan