GNU bug report logs - #78917
feature/igc [PATCH] Avoid chaining finalizers together

Previous Next

To reply to this bug, email your comments to 78917 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Helmut Eller <eller.helmut <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: feature/igc [PATCH] Avoid chaining finalizers together
Date: Sat, 28 Jun 2025 06:42:45 +0200

[Message part 1 (text/plain, inline)]

Lisp_Finalizers are currently chained together in a doubly linked list.
This prevents them from being collected.  I propose that we simply don't
use this list with MPS.

The first patch is minimalistic: it only skips finalizer_insert when
creating finalizers.

The second patch is more thorough: it #ifdefs away the prev/next fields
entirely.

[0001-Avoid-chaining-finalizers-together.patch (text/x-diff, attachment)]

[0002-For-MPS-ifdef-away-the-prev-next-fields-in-Lisp_Fina.patch (text/x-diff, attachment)]

Message #8 received at 78917 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Helmut Eller <eller.helmut <at> gmail.com>,
 Daniel Colascione <dancol <at> dancol.org>
Cc: 78917 <at> debbugs.gnu.org
Subject: Re: bug#78917: feature/igc [PATCH] Avoid chaining finalizers together
Date: Sat, 28 Jun 2025 10:24:38 +0300

> From: Helmut Eller <eller.helmut <at> gmail.com>
> Date: Sat, 28 Jun 2025 06:42:45 +0200
> 
> Lisp_Finalizers are currently chained together in a doubly linked list.
> This prevents them from being collected.  I propose that we simply don't
> use this list with MPS.
> 
> The first patch is minimalistic: it only skips finalizer_insert when
> creating finalizers.
> 
> The second patch is more thorough: it #ifdefs away the prev/next fields
> entirely.

Thanks.

Daniel, could you please tell what was the intended purpose of
chaining the finalizers?

Message #11 received at 78917 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: 78917 <at> debbugs.gnu.org
Subject: Re: bug#78917: feature/igc [PATCH] Avoid chaining finalizers together
Date: Sat, 28 Jun 2025 08:30:49 +0000

"Helmut Eller" <eller.helmut <at> gmail.com> writes:

> Lisp_Finalizers are currently chained together in a doubly linked list.
> This prevents them from being collected.  I propose that we simply don't
> use this list with MPS.

See https://lists.gnu.org/archive/html/emacs-devel/2025-05/msg00468.html
and bug#77338.

The way finalizers are currently run on the master branch when they
appear in weak tables, even if there is no chance that the entry that
references them is ever collected, makes them pretty much unusable on
that branch, in my book.  Making them usable with --with-mps=yes isn't
much of a priority for me, TBH, as code using them would break when we
compile without MPS support.

(I considered using finalizers for font finalization; it works, but
would break if a strong reference to the font is mistaken for a weak one
and fails to keep alive the font).

Pip

Message #14 received at 78917 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: 78917 <at> debbugs.gnu.org
Subject: Re: bug#78917: feature/igc [PATCH] Avoid chaining finalizers together
Date: Sat, 28 Jun 2025 09:14:12 +0000

"Helmut Eller" <eller.helmut <at> gmail.com> writes:

> Lisp_Finalizers are currently chained together in a doubly linked list.
> This prevents them from being collected.  I propose that we simply don't
> use this list with MPS.
>
> The first patch is minimalistic: it only skips finalizer_insert when
> creating finalizers.
>
> The second patch is more thorough: it #ifdefs away the prev/next fields
> entirely.

Looks good, except for the addition of igc--process-messages;
igc--collect should process *all* messages before returning, and
exposing a function that processes only some of the messages doesn't
seem useful to me.

The test probably should be rewritten to something like this:

;; Ensure finalizers are collected and bug#78917 doesn't reappear.
(ert-deftest test-two-finalizers ()
  ;; this test will usually succeed, but might fail due to unpredictable
  ;; references to the finalizer on the C stack or elsewhere.
  :tags '(:igc :unstable)
  (let* ((garbage-collection-messages t)
         (finalizer-1-called nil)
         (finalizer-2-called nil)
         (finalizer-1 (make-finalizer (lambda (setq finalizer-1-called t))))
         (finalizer-2 (make-finalizer (lambda (setq finalizer-2-called t)))))
    (dotimes (_ 3)
      (igc--collect)
      (sleep-for 0.1))
    (should (not finalizer-1-called))
    (should (not finalizer-2-called))
    (setq finalizer-1 nil)
    (dotimes (_ 1)
      (igc--collect)
      (accept-process-output nil 1 0 t))
    (should finalizer-1-called)
    (should (not finalizer-2-called))))

Pip

Message #17 received at 78917 <at> debbugs.gnu.org (full text, mbox):

From: Helmut Eller <eller.helmut <at> gmail.com>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 78917 <at> debbugs.gnu.org
Subject: Re: bug#78917: feature/igc [PATCH] Avoid chaining finalizers together
Date: Sat, 28 Jun 2025 22:21:59 +0200

[Message part 1 (text/plain, inline)]

On Sat, Jun 28 2025, Pip Cet wrote:

> "Helmut Eller" <eller.helmut <at> gmail.com> writes:
>
>> Lisp_Finalizers are currently chained together in a doubly linked list.
>> This prevents them from being collected.  I propose that we simply don't
>> use this list with MPS.
>
> See https://lists.gnu.org/archive/html/emacs-devel/2025-05/msg00468.html

:-) 

> and bug#77338.

It seems that the problem with finalizers as value in a weak hashtable
could be fixed by calling mark_and_sweep_weak_table_contents before
queue_doomed_finalizers as in the patch below.

> The way finalizers are currently run on the master branch when they
> appear in weak tables, even if there is no chance that the entry that
> references them is ever collected, makes them pretty much unusable on
> that branch, in my book.  Making them usable with --with-mps=yes isn't
> much of a priority for me, TBH, as code using them would break when we
> compile without MPS support.

Putting finalizers in a weak hashtable sounds exotic.  I suppose there
should be tests for this.

> (I considered using finalizers for font finalization; it works, but
> would break if a strong reference to the font is mistaken for a weak one
> and fails to keep alive the font).

There is some "hardcoded" finalization code in cleanup_vector for
PVEC_FONT.  Maybe you could use that instead of Lisp_Finalizers.

Helmut

[alloc.diff (text/x-diff, attachment)]

Message #20 received at 78917 <at> debbugs.gnu.org (full text, mbox):

From: Helmut Eller <eller.helmut <at> gmail.com>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 78917 <at> debbugs.gnu.org
Subject: Re: bug#78917: feature/igc [PATCH] Avoid chaining finalizers together
Date: Sat, 28 Jun 2025 22:42:15 +0200

On Sat, Jun 28 2025, Pip Cet wrote:

> "Helmut Eller" <eller.helmut <at> gmail.com> writes:
>
>> Lisp_Finalizers are currently chained together in a doubly linked list.
>> This prevents them from being collected.  I propose that we simply don't
>> use this list with MPS.
>>
>> The first patch is minimalistic: it only skips finalizer_insert when
>> creating finalizers.
>>
>> The second patch is more thorough: it #ifdefs away the prev/next fields
>> entirely.
>
> Looks good, except for the addition of igc--process-messages;
> igc--collect should process *all* messages before returning,

Maybe.  igc-collect (the one from igc.el) could easily call
igc--process-messages, though.

> exposing a function that processes only some of the messages doesn't
> seem useful to me.

It's useful to test finalizes.  It would be more useful to have an API
to test finalizers that works for both MPS and non-MPS configurations.
A candidate is

  (progn (garbage-collect) (accept-process-output))

but I guess that a plain

  (garbage-collect)

is what most people would prefer.

> The test probably should be rewritten to something like this:
>
> ;; Ensure finalizers are collected and bug#78917 doesn't reappear.
> (ert-deftest test-two-finalizers ()
>   ;; this test will usually succeed, but might fail due to unpredictable
>   ;; references to the finalizer on the C stack or elsewhere.
>   :tags '(:igc :unstable)
>   (let* ((garbage-collection-messages t)
>          (finalizer-1-called nil)
>          (finalizer-2-called nil)
>          (finalizer-1 (make-finalizer (lambda (setq finalizer-1-called t))))
>          (finalizer-2 (make-finalizer (lambda (setq finalizer-2-called t)))))
>     (dotimes (_ 3)
>       (igc--collect)
>       (sleep-for 0.1))
>     (should (not finalizer-1-called))
>     (should (not finalizer-2-called))
>     (setq finalizer-1 nil)
>     (dotimes (_ 1)
>       (igc--collect)
>       (accept-process-output nil 1 0 t))
>     (should finalizer-1-called)
>     (should (not finalizer-2-called))))

Have you tried it?  This doesn't work for me.

Helmut

Message #23 received at 78917 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: 78917 <at> debbugs.gnu.org
Subject: Re: bug#78917: feature/igc [PATCH] Avoid chaining finalizers together
Date: Sat, 28 Jun 2025 20:52:58 +0000

"Helmut Eller" <eller.helmut <at> gmail.com> writes:

> On Sat, Jun 28 2025, Pip Cet wrote:
>
>> "Helmut Eller" <eller.helmut <at> gmail.com> writes:
>>
>>> Lisp_Finalizers are currently chained together in a doubly linked list.
>>> This prevents them from being collected.  I propose that we simply don't
>>> use this list with MPS.
>>>
>>> The first patch is minimalistic: it only skips finalizer_insert when
>>> creating finalizers.
>>>
>>> The second patch is more thorough: it #ifdefs away the prev/next fields
>>> entirely.
>>
>> Looks good, except for the addition of igc--process-messages;
>> igc--collect should process *all* messages before returning,
>
> Maybe.  igc-collect (the one from igc.el) could easily call
> igc--process-messages, though.

That would work as well, if igc--process-messages processed all
messages (or returned to the caller some indication that it failed to do
so).

>> exposing a function that processes only some of the messages doesn't
>> seem useful to me.
>
> It's useful to test finalizes.  It would be more useful to have an API
> to test finalizers that works for both MPS and non-MPS configurations.

As you can't reliably test finalizers, it's not a huge problem if trying
to do so anyway requires some more code.

> but I guess that a plain
>
>   (garbage-collect)
>
> is what most people would prefer.

Hmm.  I think (garbage-collect) should become a nop in MPS builds.

>> The test probably should be rewritten to something like this:
>>
>> ;; Ensure finalizers are collected and bug#78917 doesn't reappear.
>> (ert-deftest test-two-finalizers ()
>>   ;; this test will usually succeed, but might fail due to unpredictable
>>   ;; references to the finalizer on the C stack or elsewhere.
>>   :tags '(:igc :unstable)
>>   (let* ((garbage-collection-messages t)
>>          (finalizer-1-called nil)
>>          (finalizer-2-called nil)
>>          (finalizer-1 (make-finalizer (lambda (setq finalizer-1-called t))))
>>          (finalizer-2 (make-finalizer (lambda (setq finalizer-2-called t)))))
>>     (dotimes (_ 3)
>>       (igc--collect)
>>       (sleep-for 0.1))
>>     (should (not finalizer-1-called))
>>     (should (not finalizer-2-called))
>>     (setq finalizer-1 nil)
>>     (dotimes (_ 1)
>>       (igc--collect)
>>       (accept-process-output nil 1 0 t))
>>     (should finalizer-1-called)
>>     (should (not finalizer-2-called))))
>
> Have you tried it?  This doesn't work for me.

I modified igc--collect as described above.

Pip

Message #26 received at 78917 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: 78917 <at> debbugs.gnu.org
Subject: Re: bug#78917: feature/igc [PATCH] Avoid chaining finalizers together
Date: Sat, 28 Jun 2025 21:37:11 +0000

"Helmut Eller" <eller.helmut <at> gmail.com> writes:

> On Sat, Jun 28 2025, Pip Cet wrote:
>
>> "Helmut Eller" <eller.helmut <at> gmail.com> writes:
>>
>>> Lisp_Finalizers are currently chained together in a doubly linked list.
>>> This prevents them from being collected.  I propose that we simply don't
>>> use this list with MPS.
>>
>> See https://lists.gnu.org/archive/html/emacs-devel/2025-05/msg00468.html
>
> :-)
>
>> and bug#77338.
>
> It seems that the problem with finalizers as value in a weak hashtable
> could be fixed by calling mark_and_sweep_weak_table_contents before
> queue_doomed_finalizers as in the patch below.

No, that's not the right fix: a finalizer function can still refer to
weak hash tables (the finalizer itself is not a weak object), so we
can't sweep the weak hash tables until all finalizers have been marked.

This is solvable, but it requires more effort than exchanging those two
calls.

However, I'm not sure we should fix the traditional GC code to be better
than what MPS can do :-)

>> The way finalizers are currently run on the master branch when they
>> appear in weak tables, even if there is no chance that the entry that
>> references them is ever collected, makes them pretty much unusable on
>> that branch, in my book.  Making them usable with --with-mps=yes isn't
>> much of a priority for me, TBH, as code using them would break when we
>> compile without MPS support.
>
> Putting finalizers in a weak hashtable sounds exotic.  I suppose there
> should be tests for this.

Putting font objects in a weak hash table doesn't seem exotic to me.

>> (I considered using finalizers for font finalization; it works, but
>> would break if a strong reference to the font is mistaken for a weak one
>> and fails to keep alive the font).
>
> There is some "hardcoded" finalization code in cleanup_vector for
> PVEC_FONT.  Maybe you could use that instead of Lisp_Finalizers.

The point was to get rid of that (unstable and problematic) code :-)

Pip

Message #29 received at 78917 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Colascione <dancol <at> dancol.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78917 <at> debbugs.gnu.org, Helmut Eller <eller.helmut <at> gmail.com>
Subject: Re: bug#78917: feature/igc [PATCH] Avoid chaining finalizers together
Date: Sun, 29 Jun 2025 14:20:38 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Helmut Eller <eller.helmut <at> gmail.com>
>> Date: Sat, 28 Jun 2025 06:42:45 +0200
>> 
>> Lisp_Finalizers are currently chained together in a doubly linked list.
>> This prevents them from being collected.  I propose that we simply don't
>> use this list with MPS.
>> 
>> The first patch is minimalistic: it only skips finalizer_insert when
>> creating finalizers.
>> 
>> The second patch is more thorough: it #ifdefs away the prev/next fields
>> entirely.
>
> Thanks.
>
> Daniel, could you please tell what was the intended purpose of
> chaining the finalizers?

How else do you sweep the unreachable ones at the end of GC?

Message #32 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Daniel Colascione <dancol <at> dancol.org>
To: Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text
 editors" <bug-gnu-emacs <at> gnu.org>
Cc: 78917 <at> debbugs.gnu.org, Pip Cet <pipcet <at> protonmail.com>,
 Helmut Eller <eller.helmut <at> gmail.com>
Subject: Re: bug#78917: feature/igc [PATCH] Avoid chaining finalizers together
Date: Sun, 29 Jun 2025 14:28:02 -0400

Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org> writes:

> "Helmut Eller" <eller.helmut <at> gmail.com> writes:
>
>> On Sat, Jun 28 2025, Pip Cet wrote:
>>
>>> "Helmut Eller" <eller.helmut <at> gmail.com> writes:
>>>
>>>> Lisp_Finalizers are currently chained together in a doubly linked list.
>>>> This prevents them from being collected.  I propose that we simply don't
>>>> use this list with MPS.
>>>
>>> See https://lists.gnu.org/archive/html/emacs-devel/2025-05/msg00468.html
>>
>> :-)
>>
>>> and bug#77338.
>>
>> It seems that the problem with finalizers as value in a weak hashtable
>> could be fixed by calling mark_and_sweep_weak_table_contents before
>> queue_doomed_finalizers as in the patch below.

Ouch.  That branch of the discussion of bug#77338 slipped past me.
Thanks for bringing it up.

Gut feeling is that the solution is some kind of iterated walk over
finalizers and weak tables until we reach a fixed point?  Let me think
it over.

>
> No, that's not the right fix: a finalizer function can still refer to
> weak hash tables (the finalizer itself is not a weak object), so we
> can't sweep the weak hash tables until all finalizers have been marked.
>
> This is solvable, but it requires more effort than exchanging those two
> calls.
>
> However, I'm not sure we should fix the traditional GC code to be better
> than what MPS can do :-)

Emacs 31 is going to live a long time and igc won't be its default GC.

Message #38 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Daniel Colascione <dancol <at> dancol.org>
Cc: 78917 <at> debbugs.gnu.org, "Pip Cet via \"Bug reports for GNU Emacs,
 the Swiss army knife of text editors\"" <bug-gnu-emacs <at> gnu.org>,
 Helmut Eller <eller.helmut <at> gmail.com>
Subject: Re: bug#78917: feature/igc [PATCH] Avoid chaining finalizers together
Date: Sun, 29 Jun 2025 19:02:27 +0000

"Daniel Colascione" <dancol <at> dancol.org> writes:

> Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org> writes:
>
>> "Helmut Eller" <eller.helmut <at> gmail.com> writes:
>>
>>> On Sat, Jun 28 2025, Pip Cet wrote:
>>>
>>>> "Helmut Eller" <eller.helmut <at> gmail.com> writes:
>>>>
>>>>> Lisp_Finalizers are currently chained together in a doubly linked list.
>>>>> This prevents them from being collected.  I propose that we simply don't
>>>>> use this list with MPS.
>>>>
>>>> See https://lists.gnu.org/archive/html/emacs-devel/2025-05/msg00468.html
>>>
>>> :-)
>>>
>>>> and bug#77338.
>>>
>>> It seems that the problem with finalizers as value in a weak hashtable
>>> could be fixed by calling mark_and_sweep_weak_table_contents before
>>> queue_doomed_finalizers as in the patch below.
>
> Ouch.  That branch of the discussion of bug#77338 slipped past me.
> Thanks for bringing it up.

> Gut feeling is that the solution is some kind of iterated walk over
> finalizers and weak tables until we reach a fixed point?  Let me think
> it over.

Thanks, that'd be great!

My approach was to do the combined iteration by keeping finalizers in a
weak hash table (a weak set).

1. mark all weak hash tables, including the finalizer table, until we
hit a fixed point
2. condemn unmarked finalizers; remember that somewhere
3. mark all condemned finalizers strongly
4. repeat step 1
5. sweep unmarked weak hash table entries (there are none in the
finalizer table)
6. run condemned finalizers and remove them from the table

However, I admit that using a weak hash table to keep the finalizers in
is problematic, because we might have to shrink it at some point.  Also,
no working code here, so maybe my approach was entirely wrong.

If we can fix this, we could change font finalization to use Lisp
finalizers, I think.

>> No, that's not the right fix: a finalizer function can still refer to
>> weak hash tables (the finalizer itself is not a weak object), so we
>> can't sweep the weak hash tables until all finalizers have been marked.
>>
>> This is solvable, but it requires more effort than exchanging those two
>> calls.
>>
>> However, I'm not sure we should fix the traditional GC code to be better
>> than what MPS can do :-)
>
> Emacs 31 is going to live a long time and igc won't be its default GC.

Agreed.  I'm just saying we don't need to define to the last detail how
finalizers behave wrt weak hash tables, it's better if there is some
slack there so other GC implementations can do their job.

Similarly, we should probably deprecate key-and-value strongness in
Emacs generally, and remove the hack that makes it "work" in
feature/igc.

Pip

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.