Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[Stefan Monnier]

>   Do not mark objects from deleted buffers, windows and frames.

I'm not sure this is safe.  Such deleted objects are still perfectly
live from the memory-allocation point of view, and while some operations
on them are disallowed "for ever", there are others that might still be
permitted and which may still access internal data.

E.g. you might still be able to get (window|frame)-parameters of
a deleted (window|frame).

IOW, it adds lines of code, makes the invariants more complex (in ways
which I'm not sure is currently ensured by the rest of the code) and the
benefits aren't obvious at all.

I don't think scanning those objects can take a noticeable amount of
time, so the only potential issue is holding on to data that can never
be used again, in which case I'd much prefer changing
kill-buffer/delete-(window|frame) so they set the various fields to
NULL/nil.  Which is a much safer change.


        Stefan

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[Stefan Monnier]

>> Do not mark objects from deleted buffers, windows and frames.
> I'm not sure this is safe.  Such deleted objects are still perfectly
> live from the memory-allocation point of view, and while some operations
> on them are disallowed "for ever", there are others that might still be
> permitted and which may still access internal data.

IIRC deleted windows can get resurrected via set-window-configuration.


        Stefan

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[Dmitry Antipov]

On 09/05/2012 10:24 PM, Stefan Monnier wrote:

> I'm not sure this is safe.  Such deleted objects are still perfectly
> live from the memory-allocation point of view, and while some operations
> on them are disallowed "for ever", there are others that might still be
> permitted and which may still access internal data.

IMHO such an operations shouldn't make any assumptions about internal fields
of the deleted objects. The only possible exception is NILP (obj->field) since
this may be used to distinguish between live and dead objects (as we have
now for the buffers and windows).

> E.g. you might still be able to get (window|frame)-parameters of
> a deleted (window|frame).

I'm pretty sure that this is invalid and should be fixed.

The only important exception is saving/restoring window configurations.
Strictly speaking, if the window configuration is recorded in saved_window_data,
such a window is not deleted. Ideally, struct window should have a bit
indicating that it's configuration is recorded so such a window can be
distinguished from the really dead windows; but this requires a kind of
finalization for saved_window_data objects (when such an object dies,
it's finalizer should clear corresponding bits in all recorded windows).
Since we can't implement this just now, I'm reverting window marking;
but I think that I'll spent more time on this :-).

> IOW, it adds lines of code, makes the invariants more complex (in ways
> which I'm not sure is currently ensured by the rest of the code) and the
> benefits aren't obvious at all.

Hm. For example, killed buffers may sit in all_buffers for a while, and still
have from tens to thousands reachable objects per buffer (although I didn't
check whether these objects are reachable only from this dead buffer).

> I don't think scanning those objects can take a noticeable amount of
> time, so the only potential issue is holding on to data that can never
> be used again, in which case I'd much prefer changing
> kill-buffer/delete-(window|frame) so they set the various fields to
> NULL/nil.  Which is a much safer change.

I agree about the safety, but: 1) this is slower; 2) IMHO this is conceptually
wrong and 3) it still has it's own traps (for example, if we set all Lisp_Objects
slot of the deleted window to Qnil, set-window-configuration will not work since
it assumes that the restored window has valid marker objects at W->start and W->pointm).

Dmitry

RE: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[Herring, Davis]

> but this requires a kind of
> finalization for saved_window_data objects (when such an object dies,
> it's finalizer should clear corresponding bits in all recorded windows).

Not that you're implementing this just now, but you'd have to use a reference count, since more than one window configuration could refer to a window.

Davis

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[martin rudalics]

>>   Do not mark objects from deleted buffers, windows and frames.
> 
> I'm not sure this is safe.

It is not.

martin

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[martin rudalics]

 >> E.g. you might still be able to get (window|frame)-parameters of
 >> a deleted (window|frame).
 >
 > I'm pretty sure that this is invalid and should be fixed.

Yes.  It won't be allowed any more for windows.

 > Since we can't implement this just now, I'm reverting window marking;
 > but I think that I'll spent more time on this :-).

... and please tell the people from bug 12251 about it ;-)

 > Hm. For example, killed buffers may sit in all_buffers for a while, and
 > still
 > have from tens to thousands reachable objects per buffer (although I didn't
 > check whether these objects are reachable only from this dead buffer).

Could you elaborate?

 > I agree about the safety, but: 1) this is slower; 2) IMHO this is
 > conceptually
 > wrong and 3) it still has it's own traps (for example, if we set all
 > Lisp_Objects
 > slot of the deleted window to Qnil, set-window-configuration will not
 > work since
 > it assumes that the restored window has valid marker objects at W->start
 > and W->pointm).

Marker objects belong to the buffer.  The start and pointm fields are
regenerated from the saved window structure.  Please elaborate.

martin

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[Dmitry Antipov]

On 09/06/2012 11:28 AM, martin rudalics wrote:

>  > Since we can't implement this just now, I'm reverting window marking;
>  > but I think that I'll spent more time on this :-).
>
> ... and please tell the people from bug 12251 about it ;-)

OK

>  > Hm. For example, killed buffers may sit in all_buffers for a while, and
>  > still
>  > have from tens to thousands reachable objects per buffer (although I didn't
>  > check whether these objects are reachable only from this dead buffer).
>
> Could you elaborate?

Elaborate what? I tried to count calls to mark_object needed to mark the buffer
and all data referenced from it, and it may be from 60-70 to a few thousands or
even more; some of these calls are no-ops since corresponding objects may be
marked already or doesn't need marking at all.

> Marker objects belong to the buffer.

Not all - make_window attaches two new markers to W->start and W->pointm; when W
is set up to display the buffer B (set_window_buffer or maybe somewhere else),
these markers are changed from 'point to nowhere' to 'point into B'.

> The start and pointm fields are
> regenerated from the saved window structure.

Their member values are regenerated assuming that W->start and W->pointm itself
are valid marker objects:

Fset_window_configuration:
...
else if (!NILP (BVAR (XBUFFER (p->buffer), name)))
   /* If saved buffer is alive, install it.  */
   {
     wset_buffer (w, p->buffer);
     w->start_at_line_beg = !NILP (p->start_at_line_beg);
     set_marker_restricted (w->start, p->start, w->buffer);  /* here */
     set_marker_restricted (w->pointm, p->pointm,            /* and here */
                            w->buffer);
     Fset_marker (BVAR (XBUFFER (w->buffer), mark),
                  p->mark, w->buffer);

This will throw an error if start/pointm aren't markers, or crash
if they're invalid or uninitialized; if we can't trust the values
stored in W's fields, we should use Fcopy_marker instead.

Dmitry

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[Stefan Monnier]

> IMHO such an operations shouldn't make any assumptions about internal
> fields of the deleted objects.  The only possible exception is NILP
> (obj->field) since this may be used to distinguish between live and
> dead objects (as we have now for the buffers and windows).

No, there can be perfectly valid reasons to access
a deleted/killed object.

>> E.g. you might still be able to get (window|frame)-parameters of
>> a deleted (window|frame).
> I'm pretty sure that this is invalid and should be fixed.

I don't see anything that needs fixing here.  On the contrary.  It can
be very useful to keep a reference to a frame/window, and when you need
to display data, see if it's still live, and if not create a new one,
initializing some of its parameters from the old deleted one.

> The only important exception is saving/restoring window configurations.
> Strictly speaking, if the window configuration is recorded in
> saved_window_data, such a window is not deleted.  Ideally, struct
> window should have a bit indicating that it's configuration is
> recorded so such a window can be distinguished from the really dead
> windows;

In 99% of the cases, it's easy to distinguish: one can be GC'd and the
other can't.

>> IOW, it adds lines of code, makes the invariants more complex (in ways
>> which I'm not sure is currently ensured by the rest of the code) and the
>> benefits aren't obvious at all.
> Hm.  For example, killed buffers may sit in all_buffers for a while,
> and still have from tens to thousands reachable objects per buffer
> (although I didn't check whether these objects are reachable only from
> this dead buffer).

So, that's the problem you're attacking.  Fine, but please:
1- Check how much extra data is really kept live because of this.
   As you mention, it may be a lot less than it appears at first.
2- Check how much longer it's kept alive.
4- Check which fields of the buffer keep that data alive.
3- Check why the buffer itself is kept alive: maybe the better fix is to make
   it so the buffer can be GC'd.

>> I don't think scanning those objects can take a noticeable amount of
>> time, so the only potential issue is holding on to data that can never
>> be used again, in which case I'd much prefer changing
>> kill-buffer/delete-(window|frame) so they set the various fields to
>> NULL/nil.  Which is a much safer change.

> I agree about the safety, but:
> 1) this is slower;

Yes, it could be slower, tho I expect there are only a few slots that
would need such a treatment, so I don't think it would be noticeable.

> 2) IMHO this is conceptually wrong and

I strongly disagree.  I think you have an incorrect understanding of
what a "deleted/killed" object is.  It really isn't dead at all.

> 3) it still has it's own traps.

Obviously, we don't want to NULL/nil all fields blindly.  We want to do
it parsimoniously, so that we only do it for fields which we know can
safely be set this way, and ideally only for fields which do hold on to
too much data (e.g. not for fields which only contain numbers anyway).


        Stefan

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[Stefan Monnier]

>> I'm pretty sure that this is invalid and should be fixed.
> Yes.  It won't be allowed any more for windows.

That's sad.


        Stefan

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[martin rudalics]

 >>  > Hm. For example, killed buffers may sit in all_buffers for a while,
 >> and
 >>  > still
 >>  > have from tens to thousands reachable objects per buffer (although
 >> I didn't
 >>  > check whether these objects are reachable only from this dead buffer).
 >>
 >> Could you elaborate?
 >
 > Elaborate what?

Stefan answered that for me.

 >> Marker objects belong to the buffer.
 >
 > Not all

Unless they point nowhere.  Or what do you mean here?

 > - make_window attaches two new markers to W->start and
 > W->pointm; when W
 > is set up to display the buffer B (set_window_buffer or maybe somewhere
 > else),
 > these markers are changed from 'point to nowhere' to 'point into B'.

Hence they belong to buffer B now.

 >> The start and pointm fields are
 >> regenerated from the saved window structure.
 >
 > Their member values are regenerated assuming that W->start and W->pointm
 > itself
 > are valid marker objects:
 > Fset_window_configuration:
 > ...
 > else if (!NILP (BVAR (XBUFFER (p->buffer), name)))
 >   /* If saved buffer is alive, install it.  */
 >   {
 >     wset_buffer (w, p->buffer);
 >     w->start_at_line_beg = !NILP (p->start_at_line_beg);
 >     set_marker_restricted (w->start, p->start, w->buffer);  /* here */
 >     set_marker_restricted (w->pointm, p->pointm,            /* and here */
 >                            w->buffer);
 >     Fset_marker (BVAR (XBUFFER (w->buffer), mark),
 >                  p->mark, w->buffer);
 >
 > This will throw an error if start/pointm aren't markers, or crash
 > if they're invalid or uninitialized; if we can't trust the values
 > stored in W's fields, we should use Fcopy_marker instead.

I miss you.  Do you mean that

(let ((window (split-window)))
   (set-window-buffer window (get-buffer-create "*foo*"))
   (sit-for 1)
   (save-window-excursion
     (delete-window window)
     (kill-buffer "*foo*")
     (sit-for 1)))

should crash?

martin

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[martin rudalics]

 > No, there can be perfectly valid reasons to access
 > a deleted/killed object.

Its components?

 > I don't see anything that needs fixing here.  On the contrary.  It can
 > be very useful to keep a reference to a frame/window, and when you need
 > to display data, see if it's still live, and if not create a new one,
 > initializing some of its parameters from the old deleted one.

Do you really do that?  Currently, the window must be valid for that
purpose.  So some of your code must be broken.

 > I strongly disagree.  I think you have an incorrect understanding of
 > what a "deleted/killed" object is.  It really isn't dead at all.

IMHO WINDOWP is often used where WINDOW_LIVE_P should be used instead.

 > Obviously, we don't want to NULL/nil all fields blindly.  We want to do
 > it parsimoniously, so that we only do it for fields which we know can
 > safely be set this way, and ideally only for fields which do hold on to
 > too much data (e.g. not for fields which only contain numbers anyway).

Fully agreed.

martin

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[martin rudalics]

>>> I'm pretty sure that this is invalid and should be fixed.
>> Yes.  It won't be allowed any more for windows.
> 
> That's sad.

I can revert that.

martin

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[Dmitry Antipov]

On 09/06/2012 04:52 PM, Stefan Monnier wrote:
> I don't see anything that needs fixing here.  On the contrary.  It can
> be very useful to keep a reference to a frame/window, and when you need
> to display data, see if it's still live, and if not create a new one,
> initializing some of its parameters from the old deleted one.

Yes, this is an contrary solution which has it's own advantages; but
currently we have nothing similar neither for buffers nor for frames.

>> The only important exception is saving/restoring window configurations.
>> Strictly speaking, if the window configuration is recorded in
>> saved_window_data, such a window is not deleted.  Ideally, struct
>> window should have a bit indicating that it's configuration is
>> recorded so such a window can be distinguished from the really dead
>> windows;
>
> In 99% of the cases, it's easy to distinguish: one can be GC'd and the
> other can't.

 From GC's point of view; if you look at window pointer and W->hchild,
W->vchild and W->buffer are all Qnil, you still don't know whether W
is recorded in some window configuration object.

>> Hm.  For example, killed buffers may sit in all_buffers for a while,
>> and still have from tens to thousands reachable objects per buffer
>> (although I didn't check whether these objects are reachable only from
>> this dead buffer).
>
> So, that's the problem you're attacking.  Fine, but please:

Yes, an idea comes after looking at dead buffers which are still chained
among live ones at all_buffers.

> 1- Check how much extra data is really kept live because of this.
>     As you mention, it may be a lot less than it appears at first.
> 2- Check how much longer it's kept alive.
> 4- Check which fields of the buffer keep that data alive.
> 3- Check why the buffer itself is kept alive: maybe the better fix is to make
>     it so the buffer can be GC'd.

OK

>> 2) IMHO this is conceptually wrong and
>
> I strongly disagree.  I think you have an incorrect understanding of
> what a "deleted/killed" object is.  It really isn't dead at all.

Hm... it was definitely wrong for the windows because deleted window
may be "a little more alive" than deleted buffer or frame; but I still think
that treating "deleted/killed" object as "dead enough to not
look inside too much" is good enough for the rest.

Dmitry

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[Stefan Monnier]

> Do you really do that?  Currently, the window must be valid for that
> purpose.  So some of your code must be broken.

I definitely remember doing that for processes and see no reason why the
same reasons wouldn't push me to do the same for other objects.
Of course, if needed I could work around this by using a hash table
with weak keys, but it would be better to make (frame|window)-parameters
work rather than force people to re-invent them differently.


        Stefan

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[Stefan Monnier]

>> In 99% of the cases, it's easy to distinguish: one can be GC'd and the
>> other can't.
> From GC's point of view; if you look at window pointer and W->hchild,
> W-> vchild and W->buffer are all Qnil, you still don't know whether W
> is recorded in some window configuration object.

If you're looking at W, that means there's a reference to it, and
assuming that window is deleted, I say there's a 99% chance that the
reference to it comes from a window-config.

> Hm... it was definitely wrong for the windows because deleted window
> may be "a little more alive" than deleted buffer or frame; but I still think
> that treating "deleted/killed" object as "dead enough to not
> look inside too much" is good enough for the rest.

Let's do it field-by-field (and by setting to field to NULL/nil, which
is a safer way to get the same result) rather than in one big jump into
the unknown.


        Stefan

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[martin rudalics]

 >> Do you really do that?  Currently, the window must be valid for that
 >> purpose.  So some of your code must be broken.
 >
 > I definitely remember doing that for processes and see no reason why the
 > same reasons wouldn't push me to do the same for other objects.
 > Of course, if needed I could work around this by using a hash table
 > with weak keys, but it would be better to make (frame|window)-parameters
 > work rather than force people to re-invent them differently.

As Chong remarked earlier, functions like `window-absolute-pixel-edges'
could crash when invoked on dead windows.  So we should look into
whether and where we want to allow functions to operate on dead windows
in order to maintain some consistency.

martin

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[martin rudalics]

 > If you're looking at W, that means there's a reference to it, and
 > assuming that window is deleted, I say there's a 99% chance that the
 > reference to it comes from a window-config.

I don't know of an easy way to get a dead window's identity from a
stored window configuration.  So I supoose that in 99% of all cases the
reference comes from a user variable holding a reference to that window.

martin

Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames.

[Stefan Monnier]

> I don't know of an easy way to get a dead window's identity from a
> stored window configuration.

You mean for Elisp to get it?  No, you can't, but that was not the
question.  The question was how the GC can get it, which is easy, it's
in the `window' slot of the `saved_window' struct:

   /* This is saved as a Lisp_Vector.  */
   struct saved_window
   {
     struct vectorlike_header header;
   
     Lisp_Object window, buffer, start, pointm, mark;
     Lisp_Object left_col, top_line, total_cols, total_lines;
   ...


-- Stefan

Reachable killed buffers [Was: Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames]

[Dmitry Antipov]

I found two places where the killed buffer may survive for a while
and so create some unneeded pressure to GC:

1) 'where' slot of Lisp_Buffer_Local_Value, which has local_if_set 0.
    When the buffer is killed, swap_out_buffer_local_variables can't see
    this BLV's symbol because it's not in buffer's local_var_alist.
    Shouldn't we call to swap_in_global_binding for such an "orphaned" BLVs?

2) 'prev_buffers' and 'next_buffers' of struct window. When the buffer
    is killed, replace_buffer_in_windows should remove it from these lists;
    but window-list-1 returns only live windows, so we may end up with
    dead window with a long list of killed buffers referenced from prev_buffers
    and/or next_buffers.

Dmitry

Re: Reachable killed buffers [Was: Re: [Emacs-diffs] /srv/bzr/emacs/trunk r109890: Do not mark objects from deleted buffers, windows and frames]

[Stefan Monnier]

> I found two places where the killed buffer may survive for a while
> and so create some unneeded pressure to GC:
> 1) 'where' slot of Lisp_Buffer_Local_Value, which has local_if_set 0.
>    When the buffer is killed, swap_out_buffer_local_variables can't see
>    this BLV's symbol because it's not in buffer's local_var_alist.
>    Shouldn't we call to swap_in_global_binding for such an "orphaned" BLVs?

Hmm... how can we find those?  I guess we could handle it during GC when
we mark the BLV.

> 2) 'prev_buffers' and 'next_buffers' of struct window.  When the buffer
>    is killed, replace_buffer_in_windows should remove it from these lists;
>    but window-list-1 returns only live windows, so we may end up with
>    dead window with a long list of killed buffers referenced from
>    prev_buffers and/or next_buffers.

I think here as well the problem is how to find those windows, and again
I think the easiest is to do it during GC.


        Stefan

Re: Reachable killed buffers

[Dmitry Antipov]

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

On 09/10/2012 05:25 PM, Stefan Monnier wrote:

> Hmm... how can we find those?  I guess we could handle it during GC when
> we mark the BLV.
[...skip...]
> I think here as well the problem is how to find those windows, and again
> I think the easiest is to do it during GC.

Like this?

Dmitry


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

=== modified file 'src/alloc.c'
--- src/alloc.c	2012-09-07 07:24:08 +0000
+++ src/alloc.c	2012-09-10 15:04:15 +0000
@@ -6009,6 +6009,12 @@
 	    {
 	      struct window *w = (struct window *) ptr;
 
+	      /* Lisp code removes killed buffers from buffer lists
+		 of live windows.  For dead windows, we do it here
+		 in attempt to help GC to reclaim killed buffers.  */
+	      if (!WINDOW_VALID_P (obj))
+		window_drain_buffer_lists (w);
+
 	      mark_vectorlike (ptr);
 	      /* Mark glyphs for leaf windows.  Marking window
 		 matrices is sufficient because frame matrices
@@ -6081,10 +6087,10 @@
 	  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.  */
+	      /* If the value is set up for a killed buffer or deleted
+		 frame, restore it's global binding.  */
+	      if (orphaned_local_binding (blv))
+		swap_in_global_binding (ptr);
 	      mark_object (blv->where);
 	      mark_object (blv->valcell);
 	      mark_object (blv->defcell);

=== modified file 'src/data.c'
--- src/data.c	2012-09-09 16:06:33 +0000
+++ src/data.c	2012-09-10 15:11:42 +0000
@@ -1020,7 +1020,18 @@
 	store_symval_forwarding (blv->fwd, blv_value (blv), NULL);
     }
 }
-\f
+
+/* True if binding is set up for a killed buffer or deleted frame.  */
+
+bool
+orphaned_local_binding (struct Lisp_Buffer_Local_Value *blv)
+{
+  Lisp_Object where = blv->where;
+
+  return ((BUFFERP (where) && NILP (BVAR (XBUFFER (where), name)))
+	  || (FRAMEP (where) && !FRAME_LIVE_P (XFRAME (where))));
+}
+
 /* Find the value of a symbol, returning Qunbound if it's not bound.
    This is helpful for code which just wants to get a variable's value
    if it has one, without signaling an error.

=== modified file 'src/lisp.h'
--- src/lisp.h	2012-09-10 01:17:23 +0000
+++ src/lisp.h	2012-09-10 14:26:18 +0000
@@ -2615,6 +2615,7 @@
 extern void set_internal (Lisp_Object, Lisp_Object, Lisp_Object, bool);
 extern void syms_of_data (void);
 extern void init_data (void);
+extern bool orphaned_local_binding (struct Lisp_Buffer_Local_Value *);
 extern void swap_in_global_binding (struct Lisp_Symbol *);
 
 /* Defined in cmds.c */

=== modified file 'src/window.c'
--- src/window.c	2012-09-05 07:18:46 +0000
+++ src/window.c	2012-09-10 14:59:00 +0000
@@ -3002,7 +3002,42 @@
   FOR_EACH_FRAME (tail, frame)
     window_loop (REPLACE_BUFFER_IN_WINDOWS_SAFELY, buffer, 1, frame);
 }
-\f
+
+/* Change alist started at ALISTPTR so it doesn't contains an items whose
+   car is a killed buffer.  */
+
+static inline void
+delete_killed_buffers (Lisp_Object *alistptr)
+{
+  Lisp_Object tail, prev, buffer;
+
+  eassert (alistptr != NULL);
+  for (tail = *alistptr, prev = Qnil; CONSP (tail); tail = XCDR (tail))
+    {
+      buffer = CAR (XCAR (tail));
+      CHECK_BUFFER (buffer);
+      if (NILP (BVAR (XBUFFER (buffer), name)))
+	{
+	  if (NILP (prev))
+	    *alistptr = XCDR (tail);
+	  else
+	    XSETCDR (prev, XCDR (tail));
+	}
+      else
+	prev = tail;
+    }
+}
+
+/* Delete killed buffers from both W's window lists.  Called during GC
+   for dead windows only.  */
+
+void
+window_drain_buffer_lists (struct window *w)
+{
+  delete_killed_buffers (&w->prev_buffers);
+  delete_killed_buffers (&w->next_buffers);
+}
+
 /* If *ROWS or *COLS are too small a size for FRAME, set them to the
    minimum allowable size.  */
 

=== modified file 'src/window.h'
--- src/window.h	2012-09-05 17:05:32 +0000
+++ src/window.h	2012-09-10 14:59:27 +0000
@@ -978,6 +978,7 @@
 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);
+extern void window_drain_buffer_lists (struct window *);
 extern void init_window_once (void);
 extern void init_window (void);
 extern void syms_of_window (void);

Re: Reachable killed buffers

[Stefan Monnier]

> Like this?

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

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.  But please add comments to those functions indicating the
constraints that they need to satisfy.

>  	      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.  */
> +	      /* If the value is set up for a killed buffer or deleted
> +		 frame, restore it's global binding.  */
> +	      if (orphaned_local_binding (blv))
> +		swap_in_global_binding (ptr);

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


        Stefan

Re: Reachable killed buffers

[Stefan Monnier]

> as well.  But please add comments to those functions indicating the
> constraints that they need to satisfy.

BTW, the basic constraint goes as follows:
Assignments need to either modify objects which we have no yet marked,
or install a new value which we know is marked already (or which we're
about to mark).


        Stefan

Re: Reachable killed buffers

[Dmitry Antipov]

[-- 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.  */
 

Re: Reachable killed buffers

[Stefan Monnier]

> 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.

The part where it gets less obvious is that swap_in_global_binding
doesn't only modify the BLV struct but also the C variable pointed to by
the `fwd' field and that variable is marked from elsewhere (staticpro,
for example).
As I said, I still think it's safe because we know the values we assign
there will be marked "real soon now" (via the "mark_object
(blv->defcell)" for example).

BTW, the "FIXME: make sure it's safe during GC." you placed before the
call to swap_in_global_binding should be turned into a "BEWARE: this is
called during GC, so make sure we ..." placed within swap_in_global_binding.

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

Good point, yes, thanks for updating the comment.


        Stefan

Re: Reachable killed buffers

[martin rudalics]

+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;
+}

I'm all in favor for such a routine.  But note that users can make these
lists circular and that's why I refrained from handling them while input
is blocked.  I once intended to move related code to C and run a
hare/tortoise check but Chong didn't consider it worth the effort.
Maybe for the collector it is?

martin

Re: Reachable killed buffers

[Paul Eggert]

On 09/12/2012 01:09 AM, martin rudalics wrote:
> users can make these
> lists circular and that's why I refrained from handling them while input
> is blocked.  I once intended to move related code to C and run a
> hare/tortoise check but Chong didn't consider it worth the effort.
> Maybe for the collector it is?

It would be nicer if the collector wouldn't infinite-loop, yes.

Re: Reachable killed buffers

[Dmitry Antipov]

On 09/12/2012 05:47 PM, Paul Eggert wrote:

> It would be nicer if the collector wouldn't infinite-loop, yes.

Yes, but how the collector should handle such a lists?

Dmitry

Re: Reachable killed buffers

[Paul Eggert]

On 09/12/2012 06:59 AM, Dmitry Antipov wrote:
> how the collector should handle such a lists

The dumb answer is: it would collect them the same way that
it collects any other circular list.

Perhaps there's something smarter, but I hope the dumb answer
is good enough....

Re: Reachable killed buffers

[martin rudalics]

> Perhaps there's something smarter, but I hope the dumb answer
> is good enough....

... it is ;-)

martin

Re: Reachable killed buffers

[Dmitry Antipov]

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

On 09/12/2012 06:15 PM, martin rudalics wrote:

>> Perhaps there's something smarter, but I hope the dumb answer
>> is good enough....
>
> ... it is ;-)

What if we join marking and killed buffer removal? Thus we
can avoid the infinite-loop on circularity and double walking
through the list (one for removal and one for marking)...

Dmitry

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

=== modified file 'src/alloc.c'
--- src/alloc.c	2012-09-11 20:35:23 +0000
+++ src/alloc.c	2012-09-12 15:53:01 +0000
@@ -5865,16 +5865,16 @@
     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.  */
+/* Remove killed buffers or items whose car is a killed buffer from
+   LIST, and mark other items. Return changed LIST, which is marked.  */
 
 static Lisp_Object
-discard_killed_buffers (Lisp_Object list)
+mark_discard_killed_buffers (Lisp_Object list)
 {
   Lisp_Object *prev = &list;
-  Lisp_Object tail;
+  Lisp_Object tail = list;
 
-  for (tail = list; CONSP (tail); tail = XCDR (tail))
+  while (CONSP (tail) && !CONS_MARKED_P (XCONS (tail)))
     {
       Lisp_Object tem = XCAR (tail);
       if (CONSP (tem))
@@ -5882,7 +5882,12 @@
       if (BUFFERP (tem) && !BUFFER_LIVE_P (XBUFFER (tem)))
 	*prev = XCDR (tail);
       else
-	prev = &XCDR_AS_LVALUE (tail);
+	{
+	  CONS_MARK (tail);
+	  mark_object (XCAR (tail));
+	  prev = &XCDR_AS_LVALUE (tail);
+	}
+      tail = XCDR (tail);
     }
   return list;
 }
@@ -6030,7 +6035,7 @@
 		 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));
+		fset_buffer_list (f, mark_discard_killed_buffers (f->buffer_list));
 
 	      mark_vectorlike (ptr);
 	      mark_face_cache (f->face_cache);
@@ -6048,9 +6053,9 @@
 	      if (leaf && NILP (w->buffer))
 		{
 		  wset_prev_buffers
-		    (w, discard_killed_buffers (w->prev_buffers));
+		    (w, mark_discard_killed_buffers (w->prev_buffers));
 		  wset_next_buffers
-		    (w, discard_killed_buffers (w->next_buffers));
+		    (w, mark_discard_killed_buffers (w->next_buffers));
 		}
 
 	      mark_vectorlike (ptr);

Re: Reachable killed buffers

[martin rudalics]

> What if we join marking and killed buffer removal? Thus we
> can avoid the infinite-loop on circularity and double walking
> through the list (one for removal and one for marking)...

Looks good to me.

martin

Re: Reachable killed buffers

[Paul Eggert]

On 09/12/2012 08:59 AM, Dmitry Antipov wrote:
> -  Lisp_Object tail;
> -  for (tail = list; CONSP (tail); tail = XCDR (tail))
> +  Lisp_Object tail = list;
> +  while (CONSP (tail) && !CONS_MARKED_P (XCONS (tail)))
...
...
> +      tail = XCDR (tail);

This will keep killed buffers that happen to follow
an item on the list that is already marked for other
reasons.  Shouldn't all the killed buffers be discarded?

Also, as a minor style matter, I prefer the form of that
for-loop to the less-clear while loop.

Re: Reachable killed buffers

[Stefan Monnier]

> This will keep killed buffers that happen to follow
> an item on the list that is already marked for other
> reasons.  Shouldn't all the killed buffers be discarded?

Arguably, if (part of) that list is shared with some other data
structure, then we indeed shouldn't remove dead buffers from it.
So I think the patch would be doing the right thing.


        Stefan

Re: Reachable killed buffers

[Paul Eggert]

On 09/12/2012 08:29 PM, Stefan Monnier wrote:
> Arguably, if (part of) that list is shared with some other data
> structure, then we indeed shouldn't remove dead buffers from it.

But the patch doesn't implement that either, right?
If part of the list is shared, but the GC doesn't discover
this until after that code runs, it'll remove dead
buffers from that part.  From the user's point of
view, whether dead buffers are removed would depend
on the phase of the moon.

Re: Reachable killed buffers

[Dmitry Antipov]

On 09/13/2012 08:43 AM, Paul Eggert wrote:
> On 09/12/2012 08:29 PM, Stefan Monnier wrote:
>> Arguably, if (part of) that list is shared with some other data
>> structure, then we indeed shouldn't remove dead buffers from it.
>
> But the patch doesn't implement that either, right?
> If part of the list is shared, but the GC doesn't discover
> this until after that code runs, it'll remove dead
> buffers from that part.  From the user's point of
> view, whether dead buffers are removed would depend
> on the phase of the moon.

This is the common problem, similar to what the user may call setcdr
for some list and then found that another list is also changed.
I don't see a problem here - the user should realize that destructive
list modifications may cause some strange effects.

Dmitry

Re: Reachable killed buffers

[Paul Eggert]

On 09/12/2012 10:00 PM, Dmitry Antipov wrote:
> the user should realize that destructive
> list modifications may cause some strange effects.

If destructive list modifications have the effect of
destroying a list, that's not a strange effect -- on the
contrary, it's what the user wants.  But if the modifications
also cause buffers to be deleted seemingly at random -- sometimes
reachable buffers are deleted by the garbage collector,
and sometimes not -- that's a bit too weird for comfort.

Re: Reachable killed buffers

[Stefan Monnier]

>> Arguably, if (part of) that list is shared with some other data
>> structure, then we indeed shouldn't remove dead buffers from it.
> But the patch doesn't implement that either, right?
> If part of the list is shared, but the GC doesn't discover
> this until after that code runs, it'll remove dead
> buffers from that part.

Oops, you're right.  So, I think it's OK to remove the dead buffers
(like we do now), but indeed the proposed patch wouldn't be good because
it would sometimes remove them and sometimes not, depending on the order
in which the heap is traversed by the mark phase.


        Stefan

Re: Reachable killed buffers

[martin rudalics]

 > If part of the list is shared, but the GC doesn't discover
 > this until after that code runs, it'll remove dead
 > buffers from that part.  From the user's point of
 > view, whether dead buffers are removed would depend
 > on the phase of the moon.

I'm too silly to understand.  If a buffer on one of these lists is dead,
it simply doesn't get marked from the list, so it's possible that it's
reclaimed with the next sweep.  In addition, the cons is removed from
the list which should not harm anyone.  If there's some other live
reference to the cons around, the cons gets marked and the dead buffer
as well, and mayby some other former member of the list.  So can some
kind soul please tell me what's wrong?

martin

Re: Reachable killed buffers

[Paul Eggert]

On 09/13/2012 09:49 AM, martin rudalics wrote:
> If a buffer on one of these lists is dead, it simply doesn't get
> marked from the list, so it's possible that it's reclaimed with the
> next sweep.

Right, but as I understand it in the meantime it would still exist as
a dead buffer, and would continue to pressure the GC.  And it might
inadvertently survive the next sweep too, no?  The goal is to reclaim
all the dead buffers, not just some of them.

Re: Reachable killed buffers

[martin rudalics]

 > Right, but as I understand it in the meantime it would still exist as
 > a dead buffer, and would continue to pressure the GC.  And it might
 > inadvertently survive the next sweep too, no?

Yes.  But a user could have a reference right to the dead buffer which
is much more likely than a reference to an object of a window's previous
or next buffers.

 > The goal is to reclaim
 > all the dead buffers, not just some of them.

Unless they have strong pointers.  The ones from the windows are weak.

martin

Re: Reachable killed buffers

[Dmitry Antipov]

On 09/13/2012 09:11 PM, Paul Eggert wrote:
> Right, but as I understand it in the meantime it would still exist as
> a dead buffer, and would continue to pressure the GC.  And it might
> inadvertently survive the next sweep too, no?  The goal is to reclaim
> all the dead buffers, not just some of them.

No. If finding dead buffer pointer requires the long walk through a lot
of large objects, it's not worth doing; the goal is to find dead buffer
pointers in some "special slots" where such a pointers tends to concentrate
(like prev_buffers and next_buffers of window objects) and to hope that
the most of dead buffers are reachable just once and so can be reclaimed
if we do not reference them from such a "special slots".

Dmitry

Re: Reachable killed buffers

[Dmitry Antipov]

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

Since our discussion shows that there is no solid and bullet-proof
solution for killed buffers problem (IMHO), I would like to propose
an alternate (less effective, but safe) solution: when the window
dies, it's prev_buffers and next_buffers are set to nil, but both
buffer lists are saved in struct saved_window just for the sake of
possible resurrection. Martin said that the most of window-configuration
objects are created with 'save-window-excursion' and so they're
short-lived objects which tends to not survive next GC. So, if
deleted window isn't recorded in window-configuration object or
such an object dies quickly, we're lucky; otherwise, the only thing
we can do is to hope that recorded window will be resurrected and
Lisp code from window.el will take care about the contents of
it's buffer lists.

Dmitry


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

=== modified file 'src/alloc.c'
--- src/alloc.c	2012-09-13 05:18:26 +0000
+++ src/alloc.c	2012-09-14 11:11:54 +0000
@@ -5865,32 +5865,6 @@
     mark_buffer (buffer->base_buffer);
 }
 
-/* Remove killed buffers or items whose car is a killed buffer from
-   LIST, and mark other items. Return changed LIST, which is marked.  */
-
-static Lisp_Object
-mark_discard_killed_buffers (Lisp_Object list)
-{
-  Lisp_Object tail, *prev = &list;
-
-  for (tail = list; CONSP (tail) && !CONS_MARKED_P (XCONS (tail));
-       tail = XCDR (tail))
-    {
-      Lisp_Object tem = XCAR (tail);
-      if (CONSP (tem))
-	tem = XCAR (tem);
-      if (BUFFERP (tem) && !BUFFER_LIVE_P (XBUFFER (tem)))
-	*prev = XCDR (tail);
-      else
-	{
-	  CONS_MARK (XCONS (tail));
-	  mark_object (XCAR (tail));
-	  prev = &XCDR_AS_LVALUE (tail);
-	}
-    }
-  return list;
-}
-
 /* Determine type of generic Lisp_Object and mark it accordingly.  */
 
 void
@@ -6034,24 +6008,12 @@
 	  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, mark_discard_killed_buffers (w->prev_buffers));
-		  wset_next_buffers
-		    (w, mark_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 (leaf && w->current_matrix)
+	      if (NILP (w->hchild) && NILP (w->vchild) && w->current_matrix)
 		{
 		  mark_glyph_matrix (w->current_matrix);
 		  mark_glyph_matrix (w->desired_matrix);

=== modified file 'src/window.c'
--- src/window.c	2012-09-11 15:42:50 +0000
+++ src/window.c	2012-09-14 12:09:06 +0000
@@ -3988,6 +3988,31 @@
   return new;
 }
 
+/* Reset W's fields so WINDOW_VALID_P is non-zero for W.  */
+
+static void
+kill_window (struct window *w)
+{
+  if (!NILP (w->vchild))
+    {
+      delete_all_child_windows (w->vchild);
+      wset_vchild (w, Qnil);
+    }
+  else if (!NILP (w->hchild))
+    {
+      delete_all_child_windows (w->hchild);
+      wset_hchild (w, Qnil);
+    }
+  else if (!NILP (w->buffer))
+    {
+      unshow_buffer (w);
+      unchain_marker (XMARKER (w->pointm));
+      unchain_marker (XMARKER (w->start));
+      wset_buffer (w, Qnil);
+    }
+  wset_prev_buffers (w, Qnil);
+  wset_next_buffers (w, Qnil);
+}
 
 DEFUN ("delete-window-internal", Fdelete_window_internal, Sdelete_window_internal, 1, 1, 0,
        doc: /* Remove WINDOW from its frame.
@@ -4078,23 +4103,8 @@
       wset_next (w, Qnil);  /* Don't delete w->next too.  */
       free_window_matrices (w);
 
-      if (!NILP (w->vchild))
-	{
-	  delete_all_child_windows (w->vchild);
-	  wset_vchild (w, Qnil);
-	}
-      else if (!NILP (w->hchild))
-	{
-	  delete_all_child_windows (w->hchild);
-	  wset_hchild (w, Qnil);
-	}
-      else if (!NILP (w->buffer))
-	{
-	  unshow_buffer (w);
-	  unchain_marker (XMARKER (w->pointm));
-	  unchain_marker (XMARKER (w->start));
-	  wset_buffer (w, Qnil);
-	}
+      /* W is really dead after this.  */
+      kill_window (w);
 
       if (NILP (s->prev) && NILP (s->next))
 	  /* A matrjoshka where SIBLING has become the only child of
@@ -5486,6 +5496,7 @@
   Lisp_Object parent, prev;
   Lisp_Object start_at_line_beg;
   Lisp_Object display_table;
+  Lisp_Object prev_buffers, next_buffers;
   Lisp_Object left_margin_cols, right_margin_cols;
   Lisp_Object left_fringe_width, right_fringe_width, fringes_outside_margins;
   Lisp_Object scroll_bar_width, vertical_scroll_bar_type, dedicated;
@@ -5726,6 +5737,8 @@
 	  w->hscroll = XFASTINT (p->hscroll);
 	  w->min_hscroll = XFASTINT (p->min_hscroll);
 	  wset_display_table (w, p->display_table);
+	  wset_prev_buffers (w, p->prev_buffers);
+	  wset_next_buffers (w, p->next_buffers);
 	  wset_left_margin_cols (w, p->left_margin_cols);
 	  wset_right_margin_cols (w, p->right_margin_cols);
 	  wset_left_fringe_width (w, p->left_fringe_width);
@@ -5926,23 +5939,8 @@
   /* See Fset_window_configuration for excuse.  */
   wset_total_lines (w, w->buffer);
 
-  if (!NILP (w->vchild))
-    {
-      delete_all_child_windows (w->vchild);
-      wset_vchild (w, Qnil);
-    }
-  else if (!NILP (w->hchild))
-    {
-      delete_all_child_windows (w->hchild);
-      wset_hchild (w, Qnil);
-    }
-  else if (!NILP (w->buffer))
-    {
-      unshow_buffer (w);
-      unchain_marker (XMARKER (w->pointm));
-      unchain_marker (XMARKER (w->start));
-      wset_buffer (w, Qnil);
-    }
+  /* W is really dead after this.  */
+  kill_window (w);
 
   Vwindow_list = Qnil;
 }
@@ -6045,6 +6043,8 @@
       XSETFASTINT (p->hscroll, w->hscroll);
       XSETFASTINT (p->min_hscroll, w->min_hscroll);
       p->display_table = w->display_table;
+      p->prev_buffers = w->prev_buffers;
+      p->next_buffers = w->next_buffers;
       p->left_margin_cols = w->left_margin_cols;
       p->right_margin_cols = w->right_margin_cols;
       p->left_fringe_width = w->left_fringe_width;

Re: Reachable killed buffers

[Stefan Monnier]

> Since our discussion shows that there is no solid and bullet-proof
> solution for killed buffers problem (IMHO), I would like to propose
> an alternate (less effective, but safe) solution:

I don't think this issue deserves that much effort.
I think the original behavior (of keeping some buffers longer than
needed) wasn't that terrible and the current behavior (of sometimes
removing deleted buffers but not always) is fine as well, I suspect that
in practice those lists are pretty much never reachable from elsewhere,
so the "risk" of not removing all deleted buffers is very low.

I.e. let's keep the current code and move on to more useful issues,


        Stefan

Re: Reachable killed buffers

[martin rudalics]

 > Since our discussion shows that there is no solid and bullet-proof
 > solution for killed buffers problem (IMHO), I would like to propose
 > an alternate (less effective, but safe) solution:

The current solution is safe.

 > when the window
 > dies, it's prev_buffers and next_buffers are set to nil, but both
 > buffer lists are saved in struct saved_window just for the sake of
 > possible resurrection.

The same window can appear in different saved_window structures
simultaneously.  Would each of them store these values?

 > Martin said that the most of window-configuration
 > objects are created with 'save-window-excursion' and so they're
 > short-lived objects which tends to not survive next GC. So, if
 > deleted window isn't recorded in window-configuration object or
 > such an object dies quickly, we're lucky; otherwise, the only thing
 > we can do is to hope that recorded window will be resurrected and
 > Lisp code from window.el will take care about the contents of
 > it's buffer lists.

I can't see any problem with the present solution.  So I'd rather leave
it alone.

martin