unofficial mirror of guile-devel@gnu.org 
 help / color / mirror / Atom feed
* GC + Java finalization
@ 2021-07-03 12:05 Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-07-03 17:14 ` Maxime Devos
  2021-07-15 18:44 ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  0 siblings, 2 replies; 29+ messages in thread
From: Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library @ 2021-07-03 12:05 UTC (permalink / raw)
  To: guile-devel


[-- Attachment #1.1: Type: text/plain, Size: 856 bytes --]

Hi Guile devs,

I'm hacking on GNU LilyPond and recently wondered if Guile could run
without Java finalization that prevents collection of chains of
unreachable objects. I found that the functionality is only needed once
the first guardian is created, so it's possible to delay enabling the
mode until then. This required some fixes to free functions that
assumed dependent objects to be freed only later (see first two
patches).
The third patch delays ensuring Java finalization to scm_make_guardian,
but doesn't disable it explicitly (it's on by default in bdwgc). This
could now be done right after GC_INIT(), but it's not clear (at least
to me) whether client applications actually rely it, so I think it's
better if that's not done in Guile itself.

Please consider applying, the fixes potentially also to stable-2.2.

Thanks
Jonas

[-- Attachment #1.2: 0001-Fix-test-smob-mark.patch --]
[-- Type: text/x-patch, Size: 1854 bytes --]

From c28444893ab14bd35381c7b60545fd248cc7d877 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld <hahnjo@hahnjo.de>
Date: Fri, 2 Jul 2021 23:52:13 +0200
Subject: [PATCH 1/3] Fix test-smob-mark

* test-suite/standalone/test-smob-mark.c
  (init_smob_type): Correct size of smob type.
  (free_x): Clear smob data instead of local variable.
  (test_scm_smob_mark): Put smobs in array to ensure marking.
---
 test-suite/standalone/test-smob-mark.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/test-suite/standalone/test-smob-mark.c b/test-suite/standalone/test-smob-mark.c
index d0bb5336a..ee7e02029 100644
--- a/test-suite/standalone/test-smob-mark.c
+++ b/test-suite/standalone/test-smob-mark.c
@@ -79,7 +79,7 @@ free_x (SCM x)
   x_t *c_x;
   c_x = (x_t *) SCM_SMOB_DATA (x);
   scm_gc_free (c_x, sizeof (x_t), "x");
-  c_x = NULL;
+  SCM_SET_SMOB_DATA (x, NULL);
   return 0;
 }
 
@@ -100,14 +100,17 @@ print_x (SCM x, SCM port, scm_print_state * pstate SCM_UNUSED)
 static void
 test_scm_smob_mark ()
 {
+  SCM *smobs = scm_gc_malloc (sizeof(SCM) * SMOBS_COUNT, "smobs");
+
   int i;
   mark_call_count = 0;
   for (i = 0; i < SMOBS_COUNT; i++)
-    make_x ();
+    smobs[i] = make_x ();
   scm_gc ();
   if (mark_call_count < SMOBS_COUNT)
     {
-      fprintf (stderr, "FAIL: SMOB mark function called for each SMOB\n");
+      // Print pointer so it cannot be collected before.
+      fprintf (stderr, "FAIL: SMOB mark function called for each SMOB (smobs = %p)\n", smobs);
       exit (EXIT_FAILURE);
     }
 }
@@ -115,7 +118,7 @@ test_scm_smob_mark ()
 static void
 init_smob_type ()
 {
-  x_tag = scm_make_smob_type ("x", sizeof (x_t));
+  x_tag = scm_make_smob_type ("x", sizeof (x_t *));
   scm_set_smob_free (x_tag, free_x);
   scm_set_smob_print (x_tag, print_x);
   scm_set_smob_mark (x_tag, mark_x);
-- 
2.32.0


[-- Attachment #1.3: 0002-Avoid-matching-calls-of-scm_gc_free.patch --]
[-- Type: text/x-patch, Size: 3938 bytes --]

From 7127986755f4500c0b000f769009d1240d163468 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld <hahnjo@hahnjo.de>
Date: Fri, 2 Jul 2021 23:03:17 +0200
Subject: [PATCH 2/3] Avoid matching calls of scm_gc_free

There is no point in registering memory with the garbage collector
if it doesn't need to do its job. In fact, calling scm_gc_free in
a free function is wrong because it relies on Java finalization.

* libguile/random.c (scm_c_random_bignum): Replace matching calls of
  scm_gc_calloc and scm_gc_free.
* libguile/regex-posix.c (scm_make_regexp, regex_free): Avoid call
  of scm_gc_free in free function.
* test-suite/standalone/test-smob-mark.c (make_x, free_x): Avoid
  call of scm_gc_free in free function.
---
 THANKS                                 | 1 +
 libguile/random.c                      | 8 ++------
 libguile/regex-posix.c                 | 6 +++---
 test-suite/standalone/test-smob-mark.c | 4 ++--
 4 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/THANKS b/THANKS
index aa4877e95..786b65d1a 100644
--- a/THANKS
+++ b/THANKS
@@ -13,6 +13,7 @@ Contributors since the last release:
          Volker Grabsch
          Julian Graham
         Michael Gran
+          Jonas Hahnfeld
          Daniel Hartwig
              No Itisnt
            Neil Jerram
diff --git a/libguile/random.c b/libguile/random.c
index 63da7f5d6..ac400a9fd 100644
--- a/libguile/random.c
+++ b/libguile/random.c
@@ -324,9 +324,7 @@ scm_c_random_bignum (scm_t_rstate *state, SCM m)
   /* we know the result will be this big */
   mpz_realloc2 (SCM_I_BIG_MPZ (result), m_bits);
 
-  random_chunks =
-    (uint32_t *) scm_gc_calloc (num_chunks * sizeof (uint32_t),
-                                     "random bignum chunks");
+  random_chunks = (uint32_t *) scm_calloc (num_chunks * sizeof (uint32_t));
 
   do
     {
@@ -363,9 +361,7 @@ scm_c_random_bignum (scm_t_rstate *state, SCM m)
       /* if result >= m, regenerate it (it is important to regenerate
 	 all bits in order not to get a distorted distribution) */
     } while (mpz_cmp (SCM_I_BIG_MPZ (result), SCM_I_BIG_MPZ (m)) >= 0);
-  scm_gc_free (random_chunks,
-               num_chunks * sizeof (uint32_t),
-               "random bignum chunks");
+  free (random_chunks);
   return scm_i_normbig (result);
 }
 
diff --git a/libguile/regex-posix.c b/libguile/regex-posix.c
index a08da02db..36bb639e0 100644
--- a/libguile/regex-posix.c
+++ b/libguile/regex-posix.c
@@ -67,7 +67,7 @@ static size_t
 regex_free (SCM obj)
 {
   regfree (SCM_RGX (obj));
-  scm_gc_free (SCM_RGX (obj), sizeof(regex_t), "regex");
+  free (SCM_RGX (obj));
   return 0;
 }
 
@@ -164,7 +164,7 @@ SCM_DEFINE (scm_make_regexp, "make-regexp", 1, 0, 1,
       flag = SCM_CDR (flag);
     }
 
-  rx = scm_gc_malloc_pointerless (sizeof (regex_t), "regex");
+  rx = scm_malloc (sizeof (regex_t));
   c_pat = scm_to_locale_string (pat);
   status = regcomp (rx, c_pat,
 		    /* Make sure they're not passing REG_NOSUB;
@@ -174,7 +174,7 @@ SCM_DEFINE (scm_make_regexp, "make-regexp", 1, 0, 1,
   if (status != 0)
     {
       SCM errmsg = scm_regexp_error_msg (status, rx);
-      scm_gc_free (rx, sizeof(regex_t), "regex");
+      free (rx);
       scm_error_scm (scm_regexp_error_key,
 		     scm_from_utf8_string (FUNC_NAME),
 		     errmsg,
diff --git a/test-suite/standalone/test-smob-mark.c b/test-suite/standalone/test-smob-mark.c
index ee7e02029..d7995d6f4 100644
--- a/test-suite/standalone/test-smob-mark.c
+++ b/test-suite/standalone/test-smob-mark.c
@@ -56,7 +56,7 @@ make_x ()
   x_t *c_x;
 
   i++;
-  c_x = (x_t *) scm_gc_malloc (sizeof (x_t), "x");
+  c_x = (x_t *) scm_malloc (sizeof (x_t));
   c_x->scm_value = scm_from_int (i);
   c_x->c_value = i;
   SCM_NEWSMOB (s_x, x_tag, c_x);
@@ -78,7 +78,7 @@ free_x (SCM x)
 {
   x_t *c_x;
   c_x = (x_t *) SCM_SMOB_DATA (x);
-  scm_gc_free (c_x, sizeof (x_t), "x");
+  free (c_x);
   SCM_SET_SMOB_DATA (x, NULL);
   return 0;
 }
-- 
2.32.0


[-- Attachment #1.4: 0003-Delay-Java-finalization-for-guardians.patch --]
[-- Type: text/x-patch, Size: 1695 bytes --]

From 02204f9a57e6608a23db6944f9176059966f9fd6 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld <hahnjo@hahnjo.de>
Date: Fri, 2 Jul 2021 23:16:32 +0200
Subject: [PATCH 3/3] Delay Java finalization for guardians

It is only needed once the first guardian is created.

* libguile/guardians.c (scm_make_guardian): Move call to enable
  Java finalization from scm_init_guardians.
---
 libguile/guardians.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/libguile/guardians.c b/libguile/guardians.c
index fa8c8b8f7..357c8dbd1 100644
--- a/libguile/guardians.c
+++ b/libguile/guardians.c
@@ -306,6 +306,8 @@ guardian_apply (SCM guardian, SCM obj, SCM throw_p)
     return scm_i_get_one_zombie (guardian);
 }
 
+static scm_i_pthread_mutex_t gc_java_mutex;
+
 SCM_DEFINE (scm_make_guardian, "make-guardian", 0, 0, 0, 
 	    (),
 "Create a new guardian.  A guardian protects a set of objects from\n"
@@ -349,6 +351,11 @@ SCM_DEFINE (scm_make_guardian, "make-guardian", 0, 0, 0,
 	    "value, it is eligible to be returned from a guardian.\n")
 #define FUNC_NAME s_scm_make_guardian
 {
+  /* For guardians, we use unordered finalization `a la Java. */
+  scm_i_pthread_mutex_lock (&gc_java_mutex);
+  GC_set_java_finalization (1);
+  scm_i_pthread_mutex_unlock (&gc_java_mutex);
+
   t_guardian *g = scm_gc_malloc (sizeof (t_guardian), "guardian");
   SCM z;
 
@@ -369,8 +376,7 @@ SCM_DEFINE (scm_make_guardian, "make-guardian", 0, 0, 0,
 void
 scm_init_guardians ()
 {
-  /* We use unordered finalization `a la Java.  */
-  GC_set_java_finalization (1);
+  scm_i_pthread_mutex_init (&gc_java_mutex, NULL);
 
   tc16_guardian = scm_make_smob_type ("guardian", 0);
 
-- 
2.32.0


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-07-03 12:05 GC + Java finalization Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
@ 2021-07-03 17:14 ` Maxime Devos
  2021-07-03 17:26   ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-07-15 18:44 ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  1 sibling, 1 reply; 29+ messages in thread
From: Maxime Devos @ 2021-07-03 17:14 UTC (permalink / raw)
  To: Jonas Hahnfeld, guile-devel

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

Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library schreef op za 03-07-2021 om 14:05 [+0200]:
> Hi Guile devs,
> 

Hi, I'm not really a Guile dev but I'll respond anyway.

> I'm hacking on GNU LilyPond and recently wondered if Guile could run
> without Java finalization that prevents collection of chains of
> unreachable objects.

Do you have an example where this is a problem?
I.e., did you encounter ‘chains of unreachable objects’ that were
uncollectable, and so, where?

> I found that the functionality is only needed once
> the first guardian is created, so it's possible to delay enabling the
> mode until then. This required some fixes to free functions that
> assumed dependent objects to be freed only later (see first two
> patches).
> The third patch delays ensuring Java finalization to scm_make_guardian,
> but doesn't disable it explicitly (it's on by default in bdwgc). This
> could now be done right after GC_INIT(), but it's not clear (at least
> to me) whether client applications actually rely it, so I think it's
> better if that's not done in Guile itself.
> 
> Please consider applying, the fixes potentially also to stable-2.2.
> 
> Thanks
> Jonas

I would need to look more closely at how ‘Java-style’ finalisation
works. Some comments anyway:

(first patch)

> * test-suite/standalone/test-smob-mark.c
>   (init_smob_type): Correct size of smob type.
>   (free_x): Clear smob data instead of local variable.
>   (test_scm_smob_mark): Put smobs in array to ensure marking.
>
> -      fprintf (stderr, "FAIL: SMOB mark function called for each SMOB\n");
> +      // Print pointer so it cannot be collected before.
> +      fprintf (stderr, "FAIL: SMOB mark function called for each SMOB (smobs = %p)\n", smobs);
>        exit (EXIT_FAILURE);

Normally scm_remember_upto_here is used for that.
Also, I believe "/* */"-style comments are used customarily used in Guile
instead of "//"-style comments.

> static void
> init_smob_type ()
> {
> -  x_tag = scm_make_smob_type ("x", sizeof (x_t));
> +  x_tag = scm_make_smob_type ("x", sizeof (x_t *));

This change seems to be a fix independent of the ‘do we want Java-style finalization’
question.

(third patch)

Note that guardians are used in (ice-9 popen).
They are also used by some guile libraries (e.g. guile-fibers),
so you can't use (ice-9 popen) or any library using guardians
if Java-style finalization is undesirable.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-07-03 17:14 ` Maxime Devos
@ 2021-07-03 17:26   ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-07-03 18:49     ` Maxime Devos
  2021-07-03 18:54     ` Maxime Devos
  0 siblings, 2 replies; 29+ messages in thread
From: Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library @ 2021-07-03 17:26 UTC (permalink / raw)
  To: Maxime Devos, guile-devel

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

Am Samstag, dem 03.07.2021 um 19:14 +0200 schrieb Maxime Devos:
> Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library schreef op za 03-07-2021 om 14:05 [+0200]:
> > Hi Guile devs,
> > 
> 
> Hi, I'm not really a Guile dev but I'll respond anyway.
> 
> > I'm hacking on GNU LilyPond and recently wondered if Guile could run
> > without Java finalization that prevents collection of chains of
> > unreachable objects.
> 
> Do you have an example where this is a problem?
> I.e., did you encounter ‘chains of unreachable objects’ that were
> uncollectable, and so, where?

Sorry, I should have been clearer: Chains don't become uncollectable,
but a chain of N objects takes N collections to be completely reclaimed
(because Java finalization prepares for the possibility that a free
function makes an object live again, as Guile does for guardians). This
leads to unnecessary waste on the heap, and more work for the collector
(even though I haven't been able to measure so far).

> 
> > I found that the functionality is only needed once
> > the first guardian is created, so it's possible to delay enabling the
> > mode until then. This required some fixes to free functions that
> > assumed dependent objects to be freed only later (see first two
> > patches).
> > The third patch delays ensuring Java finalization to scm_make_guardian,
> > but doesn't disable it explicitly (it's on by default in bdwgc). This
> > could now be done right after GC_INIT(), but it's not clear (at least
> > to me) whether client applications actually rely it, so I think it's
> > better if that's not done in Guile itself.
> > 
> > Please consider applying, the fixes potentially also to stable-2.2.
> > 
> > Thanks
> > Jonas
> 
> I would need to look more closely at how ‘Java-style’ finalisation
> works. Some comments anyway:
> 
> (first patch)
> 
> > * test-suite/standalone/test-smob-mark.c
> >   (init_smob_type): Correct size of smob type.
> >   (free_x): Clear smob data instead of local variable.
> >   (test_scm_smob_mark): Put smobs in array to ensure marking.
> > 
> > -      fprintf (stderr, "FAIL: SMOB mark function called for each SMOB\n");
> > +      // Print pointer so it cannot be collected before.
> > +      fprintf (stderr, "FAIL: SMOB mark function called for each SMOB (smobs = %p)\n", smobs);
> >        exit (EXIT_FAILURE);
> 
> Normally scm_remember_upto_here is used for that.

I think I tried, but it wasn't available. Or I mistyped, not sure.

> Also, I believe "/* */"-style comments are used customarily used in Guile
> instead of "//"-style comments.
> 
> > static void
> > init_smob_type ()
> > {
> > -  x_tag = scm_make_smob_type ("x", sizeof (x_t));
> > +  x_tag = scm_make_smob_type ("x", sizeof (x_t *));
> 
> This change seems to be a fix independent of the ‘do we want Java-style finalization’
> question.

Yes, that's why it's a separate patch.

> 
> (third patch)
> 
> Note that guardians are used in (ice-9 popen).
> They are also used by some guile libraries (e.g. guile-fibers),
> so you can't use (ice-9 popen) or any library using guardians
> if Java-style finalization is undesirable.

Yes, I'm aware and that's why any constructed guardian force-enables
Java finalization. But applications might not use it, and at least for
LilyPond I'm sure it's not used.

Jonas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-07-03 17:26   ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
@ 2021-07-03 18:49     ` Maxime Devos
  2021-07-03 18:54     ` Maxime Devos
  1 sibling, 0 replies; 29+ messages in thread
From: Maxime Devos @ 2021-07-03 18:49 UTC (permalink / raw)
  To: Jonas Hahnfeld, guile-devel

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

Jonas Hahnfeld schreef op za 03-07-2021 om 19:26 [+0200]:
> Sorry, I should have been clearer: Chains don't become uncollectable,
> but a chain of N objects takes N collections to be completely reclaimed
> (because Java finalization prepares for the possibility that a free
> function makes an object live again, as Guile does for guardians). This
> leads to unnecessary waste on the heap, and more work for the collector
> (even though I haven't been able to measure so far).

Disabling Java-style finalization would be an optimisation, ok!

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-07-03 17:26   ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-07-03 18:49     ` Maxime Devos
@ 2021-07-03 18:54     ` Maxime Devos
  1 sibling, 0 replies; 29+ messages in thread
From: Maxime Devos @ 2021-07-03 18:54 UTC (permalink / raw)
  To: Jonas Hahnfeld; +Cc: guile-devel

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

Jonas Hahnfeld schreef op za 03-07-2021 om 19:26 [+0200]:

> > Normally scm_remember_upto_here is used for that.
> 
> I think I tried, but it wasn't available. Or I mistyped, not sure.

It is defined in <libguile/gc.h>.

Actually, the special case scm_remember_upto_here_1 would be prefered,
because it theoretically is more efficient.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-07-03 12:05 GC + Java finalization Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-07-03 17:14 ` Maxime Devos
@ 2021-07-15 18:44 ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-10-10 16:22   ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
                     ` (4 more replies)
  1 sibling, 5 replies; 29+ messages in thread
From: Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library @ 2021-07-15 18:44 UTC (permalink / raw)
  To: guile-devel; +Cc: Maxime Devos


[-- Attachment #1.1: Type: text/plain, Size: 1117 bytes --]

Am Samstag, dem 03.07.2021 um 14:05 +0200 schrieb Jonas Hahnfeld via
Developers list for Guile, the GNU extensibility library:
> Hi Guile devs,
> 
> I'm hacking on GNU LilyPond and recently wondered if Guile could run
> without Java finalization that prevents collection of chains of
> unreachable objects. I found that the functionality is only needed once
> the first guardian is created, so it's possible to delay enabling the
> mode until then. This required some fixes to free functions that
> assumed dependent objects to be freed only later (see first two
> patches).
> The third patch delays ensuring Java finalization to scm_make_guardian,
> but doesn't disable it explicitly (it's on by default in bdwgc). This
> could now be done right after GC_INIT(), but it's not clear (at least
> to me) whether client applications actually rely it, so I think it's
> better if that's not done in Guile itself.
> 
> Please consider applying, the fixes potentially also to stable-2.2.

I didn't receive other comments than those by Maxime, so here is an
updated version of the first patch.

Jonas

[-- Attachment #1.2: 0001-Fix-test-smob-mark.patch --]
[-- Type: text/x-patch, Size: 1694 bytes --]

From f89a8271c3cbb775a83db3e59fa66ea82f6239a1 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld <hahnjo@hahnjo.de>
Date: Fri, 2 Jul 2021 23:52:13 +0200
Subject: [PATCH 1/3] Fix test-smob-mark

* test-suite/standalone/test-smob-mark.c
  (init_smob_type): Correct size of smob type.
  (free_x): Clear smob data instead of local variable.
  (test_scm_smob_mark): Put smobs in array to ensure marking.
---
 test-suite/standalone/test-smob-mark.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/test-suite/standalone/test-smob-mark.c b/test-suite/standalone/test-smob-mark.c
index d0bb5336a..586a2b081 100644
--- a/test-suite/standalone/test-smob-mark.c
+++ b/test-suite/standalone/test-smob-mark.c
@@ -79,7 +79,7 @@ free_x (SCM x)
   x_t *c_x;
   c_x = (x_t *) SCM_SMOB_DATA (x);
   scm_gc_free (c_x, sizeof (x_t), "x");
-  c_x = NULL;
+  SCM_SET_SMOB_DATA (x, NULL);
   return 0;
 }
 
@@ -100,22 +100,25 @@ print_x (SCM x, SCM port, scm_print_state * pstate SCM_UNUSED)
 static void
 test_scm_smob_mark ()
 {
+  SCM *smobs = scm_gc_malloc (sizeof(SCM) * SMOBS_COUNT, "smobs");
+
   int i;
   mark_call_count = 0;
   for (i = 0; i < SMOBS_COUNT; i++)
-    make_x ();
+    smobs[i] = make_x ();
   scm_gc ();
   if (mark_call_count < SMOBS_COUNT)
     {
       fprintf (stderr, "FAIL: SMOB mark function called for each SMOB\n");
       exit (EXIT_FAILURE);
     }
+  scm_remember_upto_here_1 (smobs);
 }
 
 static void
 init_smob_type ()
 {
-  x_tag = scm_make_smob_type ("x", sizeof (x_t));
+  x_tag = scm_make_smob_type ("x", sizeof (x_t *));
   scm_set_smob_free (x_tag, free_x);
   scm_set_smob_print (x_tag, print_x);
   scm_set_smob_mark (x_tag, mark_x);
-- 
2.32.0


[-- Attachment #1.3: 0002-Avoid-matching-calls-of-scm_gc_free.patch --]
[-- Type: text/x-patch, Size: 3938 bytes --]

From 33af6a98c6801e7b4880d1d3f78f7e2097c2174e Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld <hahnjo@hahnjo.de>
Date: Fri, 2 Jul 2021 23:03:17 +0200
Subject: [PATCH 2/3] Avoid matching calls of scm_gc_free

There is no point in registering memory with the garbage collector
if it doesn't need to do its job. In fact, calling scm_gc_free in
a free function is wrong because it relies on Java finalization.

* libguile/random.c (scm_c_random_bignum): Replace matching calls of
  scm_gc_calloc and scm_gc_free.
* libguile/regex-posix.c (scm_make_regexp, regex_free): Avoid call
  of scm_gc_free in free function.
* test-suite/standalone/test-smob-mark.c (make_x, free_x): Avoid
  call of scm_gc_free in free function.
---
 THANKS                                 | 1 +
 libguile/random.c                      | 8 ++------
 libguile/regex-posix.c                 | 6 +++---
 test-suite/standalone/test-smob-mark.c | 4 ++--
 4 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/THANKS b/THANKS
index aa4877e95..786b65d1a 100644
--- a/THANKS
+++ b/THANKS
@@ -13,6 +13,7 @@ Contributors since the last release:
          Volker Grabsch
          Julian Graham
         Michael Gran
+          Jonas Hahnfeld
          Daniel Hartwig
              No Itisnt
            Neil Jerram
diff --git a/libguile/random.c b/libguile/random.c
index 63da7f5d6..ac400a9fd 100644
--- a/libguile/random.c
+++ b/libguile/random.c
@@ -324,9 +324,7 @@ scm_c_random_bignum (scm_t_rstate *state, SCM m)
   /* we know the result will be this big */
   mpz_realloc2 (SCM_I_BIG_MPZ (result), m_bits);
 
-  random_chunks =
-    (uint32_t *) scm_gc_calloc (num_chunks * sizeof (uint32_t),
-                                     "random bignum chunks");
+  random_chunks = (uint32_t *) scm_calloc (num_chunks * sizeof (uint32_t));
 
   do
     {
@@ -363,9 +361,7 @@ scm_c_random_bignum (scm_t_rstate *state, SCM m)
       /* if result >= m, regenerate it (it is important to regenerate
 	 all bits in order not to get a distorted distribution) */
     } while (mpz_cmp (SCM_I_BIG_MPZ (result), SCM_I_BIG_MPZ (m)) >= 0);
-  scm_gc_free (random_chunks,
-               num_chunks * sizeof (uint32_t),
-               "random bignum chunks");
+  free (random_chunks);
   return scm_i_normbig (result);
 }
 
diff --git a/libguile/regex-posix.c b/libguile/regex-posix.c
index a08da02db..36bb639e0 100644
--- a/libguile/regex-posix.c
+++ b/libguile/regex-posix.c
@@ -67,7 +67,7 @@ static size_t
 regex_free (SCM obj)
 {
   regfree (SCM_RGX (obj));
-  scm_gc_free (SCM_RGX (obj), sizeof(regex_t), "regex");
+  free (SCM_RGX (obj));
   return 0;
 }
 
@@ -164,7 +164,7 @@ SCM_DEFINE (scm_make_regexp, "make-regexp", 1, 0, 1,
       flag = SCM_CDR (flag);
     }
 
-  rx = scm_gc_malloc_pointerless (sizeof (regex_t), "regex");
+  rx = scm_malloc (sizeof (regex_t));
   c_pat = scm_to_locale_string (pat);
   status = regcomp (rx, c_pat,
 		    /* Make sure they're not passing REG_NOSUB;
@@ -174,7 +174,7 @@ SCM_DEFINE (scm_make_regexp, "make-regexp", 1, 0, 1,
   if (status != 0)
     {
       SCM errmsg = scm_regexp_error_msg (status, rx);
-      scm_gc_free (rx, sizeof(regex_t), "regex");
+      free (rx);
       scm_error_scm (scm_regexp_error_key,
 		     scm_from_utf8_string (FUNC_NAME),
 		     errmsg,
diff --git a/test-suite/standalone/test-smob-mark.c b/test-suite/standalone/test-smob-mark.c
index 586a2b081..edbb51f89 100644
--- a/test-suite/standalone/test-smob-mark.c
+++ b/test-suite/standalone/test-smob-mark.c
@@ -56,7 +56,7 @@ make_x ()
   x_t *c_x;
 
   i++;
-  c_x = (x_t *) scm_gc_malloc (sizeof (x_t), "x");
+  c_x = (x_t *) scm_malloc (sizeof (x_t));
   c_x->scm_value = scm_from_int (i);
   c_x->c_value = i;
   SCM_NEWSMOB (s_x, x_tag, c_x);
@@ -78,7 +78,7 @@ free_x (SCM x)
 {
   x_t *c_x;
   c_x = (x_t *) SCM_SMOB_DATA (x);
-  scm_gc_free (c_x, sizeof (x_t), "x");
+  free (c_x);
   SCM_SET_SMOB_DATA (x, NULL);
   return 0;
 }
-- 
2.32.0


[-- Attachment #1.4: 0003-Delay-Java-finalization-for-guardians.patch --]
[-- Type: text/x-patch, Size: 1695 bytes --]

From 6bbb76692a26090918dfed42d67c6ba42e54e034 Mon Sep 17 00:00:00 2001
From: Jonas Hahnfeld <hahnjo@hahnjo.de>
Date: Fri, 2 Jul 2021 23:16:32 +0200
Subject: [PATCH 3/3] Delay Java finalization for guardians

It is only needed once the first guardian is created.

* libguile/guardians.c (scm_make_guardian): Move call to enable
  Java finalization from scm_init_guardians.
---
 libguile/guardians.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/libguile/guardians.c b/libguile/guardians.c
index fa8c8b8f7..357c8dbd1 100644
--- a/libguile/guardians.c
+++ b/libguile/guardians.c
@@ -306,6 +306,8 @@ guardian_apply (SCM guardian, SCM obj, SCM throw_p)
     return scm_i_get_one_zombie (guardian);
 }
 
+static scm_i_pthread_mutex_t gc_java_mutex;
+
 SCM_DEFINE (scm_make_guardian, "make-guardian", 0, 0, 0, 
 	    (),
 "Create a new guardian.  A guardian protects a set of objects from\n"
@@ -349,6 +351,11 @@ SCM_DEFINE (scm_make_guardian, "make-guardian", 0, 0, 0,
 	    "value, it is eligible to be returned from a guardian.\n")
 #define FUNC_NAME s_scm_make_guardian
 {
+  /* For guardians, we use unordered finalization `a la Java. */
+  scm_i_pthread_mutex_lock (&gc_java_mutex);
+  GC_set_java_finalization (1);
+  scm_i_pthread_mutex_unlock (&gc_java_mutex);
+
   t_guardian *g = scm_gc_malloc (sizeof (t_guardian), "guardian");
   SCM z;
 
@@ -369,8 +376,7 @@ SCM_DEFINE (scm_make_guardian, "make-guardian", 0, 0, 0,
 void
 scm_init_guardians ()
 {
-  /* We use unordered finalization `a la Java.  */
-  GC_set_java_finalization (1);
+  scm_i_pthread_mutex_init (&gc_java_mutex, NULL);
 
   tc16_guardian = scm_make_smob_type ("guardian", 0);
 
-- 
2.32.0


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-07-15 18:44 ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
@ 2021-10-10 16:22   ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-11-19 12:18     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-11-19 13:13   ` Maxime Devos
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 29+ messages in thread
From: Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library @ 2021-10-10 16:22 UTC (permalink / raw)
  To: guile-devel; +Cc: Maxime Devos

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

Am Donnerstag, dem 15.07.2021 um 20:44 +0200 schrieb Jonas Hahnfeld via
Developers list for Guile, the GNU extensibility library:
> Am Samstag, dem 03.07.2021 um 14:05 +0200 schrieb Jonas Hahnfeld via
> Developers list for Guile, the GNU extensibility library:
> > Hi Guile devs,
> > 
> > I'm hacking on GNU LilyPond and recently wondered if Guile could run
> > without Java finalization that prevents collection of chains of
> > unreachable objects. I found that the functionality is only needed once
> > the first guardian is created, so it's possible to delay enabling the
> > mode until then. This required some fixes to free functions that
> > assumed dependent objects to be freed only later (see first two
> > patches).
> > The third patch delays ensuring Java finalization to scm_make_guardian,
> > but doesn't disable it explicitly (it's on by default in bdwgc). This
> > could now be done right after GC_INIT(), but it's not clear (at least
> > to me) whether client applications actually rely it, so I think it's
> > better if that's not done in Guile itself.
> > 
> > Please consider applying, the fixes potentially also to stable-2.2.
> 
> I didn't receive other comments than those by Maxime, so here is an
> updated version of the first patch.

Ping, is there anybody looking at patches sent to the mailing list?

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-10-10 16:22   ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
@ 2021-11-19 12:18     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  0 siblings, 0 replies; 29+ messages in thread
From: Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library @ 2021-11-19 12:18 UTC (permalink / raw)
  To: guile-devel; +Cc: Maxime Devos

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

Am Sonntag, dem 10.10.2021 um 18:22 +0200 schrieb Jonas Hahnfeld via
Developers list for Guile, the GNU extensibility library:
> Am Donnerstag, dem 15.07.2021 um 20:44 +0200 schrieb Jonas Hahnfeld via
> Developers list for Guile, the GNU extensibility library:
> > Am Samstag, dem 03.07.2021 um 14:05 +0200 schrieb Jonas Hahnfeld via
> > Developers list for Guile, the GNU extensibility library:
> > > Hi Guile devs,
> > > 
> > > I'm hacking on GNU LilyPond and recently wondered if Guile could run
> > > without Java finalization that prevents collection of chains of
> > > unreachable objects. I found that the functionality is only needed once
> > > the first guardian is created, so it's possible to delay enabling the
> > > mode until then. This required some fixes to free functions that
> > > assumed dependent objects to be freed only later (see first two
> > > patches).
> > > The third patch delays ensuring Java finalization to scm_make_guardian,
> > > but doesn't disable it explicitly (it's on by default in bdwgc). This
> > > could now be done right after GC_INIT(), but it's not clear (at least
> > > to me) whether client applications actually rely it, so I think it's
> > > better if that's not done in Guile itself.
> > > 
> > > Please consider applying, the fixes potentially also to stable-2.2.
> > 
> > I didn't receive other comments than those by Maxime, so here is an
> > updated version of the first patch.
> 
> Ping, is there anybody looking at patches sent to the mailing list?

Last ping before I give up...

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-07-15 18:44 ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-10-10 16:22   ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
@ 2021-11-19 13:13   ` Maxime Devos
  2021-11-19 13:32     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-11-19 13:15   ` Maxime Devos
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 29+ messages in thread
From: Maxime Devos @ 2021-11-19 13:13 UTC (permalink / raw)
  To: Jonas Hahnfeld, guile-devel

Jonas Hahnfeld schreef op do 15-07-2021 om 20:44 [+0200]:
> From 33af6a98c6801e7b4880d1d3f78f7e2097c2174e Mon Sep 17 00:00:00
> 2001
> From: Jonas Hahnfeld <hahnjo@hahnjo.de>
> Date: Fri, 2 Jul 2021 23:03:17 +0200
> Subject: [PATCH 2/3] Avoid matching calls of scm_gc_free
> 
> There is no point in registering memory with the garbage collector
> if it doesn't need to do its job. In fact, calling scm_gc_free in
> a free function is wrong because it relies on Java finalization.
> 
> * libguile/random.c (scm_c_random_bignum): Replace matching calls of
>   scm_gc_calloc and scm_gc_free.
> * libguile/regex-posix.c (scm_make_regexp, regex_free): Avoid call
>   of scm_gc_free in free function.
> * test-suite/standalone/test-smob-mark.c (make_x, free_x): Avoid
>   call of scm_gc_free in free function.
> ---
>  THANKS                                 | 1 +
>  libguile/random.c                      | 8 ++------
>  libguile/regex-posix.c                 | 6 +++---
>  test-suite/standalone/test-smob-mark.c | 4 ++--
>  4 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/THANKS b/THANKS
> index aa4877e95..786b65d1a 100644
> --- a/THANKS
> +++ b/THANKS
> @@ -13,6 +13,7 @@ Contributors since the last release:
>           Volker Grabsch
>           Julian Graham
>          Michael Gran
> +          Jonas Hahnfeld
>           Daniel Hartwig
>               No Itisnt
>             Neil Jerram
> diff --git a/libguile/random.c b/libguile/random.c
> index 63da7f5d6..ac400a9fd 100644
> --- a/libguile/random.c
> +++ b/libguile/random.c
> @@ -324,9 +324,7 @@ scm_c_random_bignum (scm_t_rstate *state, SCM m)
>    /* we know the result will be this big */
>    mpz_realloc2 (SCM_I_BIG_MPZ (result), m_bits);
>  
> -  random_chunks =
> -    (uint32_t *) scm_gc_calloc (num_chunks * sizeof (uint32_t),
> -                                     "random bignum chunks");
> +  random_chunks = (uint32_t *) scm_calloc (num_chunks * sizeof
> (uint32_t));
>  
>    do
>      {
> @@ -363,9 +361,7 @@ scm_c_random_bignum (scm_t_rstate *state, SCM m)
>        /* if result >= m, regenerate it (it is important to
> regenerate
>  	 all bits in order not to get a distorted distribution) */
>      } while (mpz_cmp (SCM_I_BIG_MPZ (result), SCM_I_BIG_MPZ (m)) >=
> 0);
> -  scm_gc_free (random_chunks,
> -               num_chunks * sizeof (uint32_t),
> -               "random bignum chunks");
> +  free (random_chunks);

From the manual (about scm_gc_calloc & friends):

‘Memory blocks allocated this way may
be released explicitly; however, this is not strictly needed, and we
recommend _not_ calling ‘scm_gc_free’.  All memory allocated with
‘scm_gc_malloc’ or ‘scm_gc_malloc_pointerless’ is automatically
reclaimed when the garbage collector no longer sees any live reference
to it(1).’

As such, I'd recommend simply dropping the scm_gc_free
(here and in other places), if the scm_gc_free was problematic
because of Java finalization reasons.


>    return scm_i_normbig (result);
>  }
>  
> diff --git a/libguile/regex-posix.c b/libguile/regex-posix.c
> index a08da02db..36bb639e0 100644
> --- a/libguile/regex-posix.c
> +++ b/libguile/regex-posix.c
> @@ -67,7 +67,7 @@ static size_t
>  regex_free (SCM obj)
>  {
>    regfree (SCM_RGX (obj));
> -  scm_gc_free (SCM_RGX (obj), sizeof(regex_t), "regex");
> +  free (SCM_RGX (obj));
>    return 0;
>  }
>  
> @@ -164,7 +164,7 @@ SCM_DEFINE (scm_make_regexp, "make-regexp", 1, 0,
> 1,
>        flag = SCM_CDR (flag);
>      }
>  
> -  rx = scm_gc_malloc_pointerless (sizeof (regex_t), "regex");
> +  rx = scm_malloc (sizeof (regex_t));

If the regex why scm_gc_malloc_pointerless -> scm_malloc?
Is rx not pointerless? You coud simply ...


> -      scm_gc_free (rx, sizeof(regex_t), "regex");
> +      free (rx);

drop the scm_gc_free AFAIK.

Greetings,
Maxime




^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-07-15 18:44 ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-10-10 16:22   ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-11-19 13:13   ` Maxime Devos
@ 2021-11-19 13:15   ` Maxime Devos
  2021-11-19 13:35     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-11-19 13:17   ` Maxime Devos
  2021-11-20  9:19   ` Maxime Devos
  4 siblings, 1 reply; 29+ messages in thread
From: Maxime Devos @ 2021-11-19 13:15 UTC (permalink / raw)
  To: Jonas Hahnfeld, guile-devel

Jonas Hahnfeld schreef op do 15-07-2021 om 20:44 [+0200]:
> +  SCM *smobs = scm_gc_malloc (sizeof(SCM) * SMOBS_COUNT, "smobs");
> +
>    int i;
>    mark_call_count = 0;
>    for (i = 0; i < SMOBS_COUNT; i++)
> -    make_x ();
> +    smobs[i] = make_x ();
>    scm_gc ();

smobs doesn't need to be protected for the whole function call,
until after the scm_gc() should be sufficient I think. 

Greetings,
Maxime




^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-07-15 18:44 ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
                     ` (2 preceding siblings ...)
  2021-11-19 13:15   ` Maxime Devos
@ 2021-11-19 13:17   ` Maxime Devos
  2021-11-19 13:38     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-11-20  9:19   ` Maxime Devos
  4 siblings, 1 reply; 29+ messages in thread
From: Maxime Devos @ 2021-11-19 13:17 UTC (permalink / raw)
  To: Jonas Hahnfeld, guile-devel

Jonas Hahnfeld schreef op do 15-07-2021 om 20:44 [+0200]:
> +  /* For guardians, we use unordered finalization `a la Java. */

Maybe add the reasons why this is only done when a guardian is created?
E.g.,

/* Don't use unordered finalization when not using guardians,
    because Java finalization prevents fast collection of chains of
unreachable objects */

Not 100% about the exact purpose of avoiding Java-style finalization,
you might want to adjust the wording somewhat ...

Greetings,
Maxime




^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 13:13   ` Maxime Devos
@ 2021-11-19 13:32     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-11-19 13:35       ` Maxime Devos
  2021-11-19 13:48       ` Maxime Devos
  0 siblings, 2 replies; 29+ messages in thread
From: Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library @ 2021-11-19 13:32 UTC (permalink / raw)
  To: Maxime Devos, guile-devel

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

Am Freitag, dem 19.11.2021 um 13:13 +0000 schrieb Maxime Devos:
> Jonas Hahnfeld schreef op do 15-07-2021 om 20:44 [+0200]:
> > From 33af6a98c6801e7b4880d1d3f78f7e2097c2174e Mon Sep 17 00:00:00
> > 2001
> > From: Jonas Hahnfeld <hahnjo@hahnjo.de>
> > Date: Fri, 2 Jul 2021 23:03:17 +0200
> > Subject: [PATCH 2/3] Avoid matching calls of scm_gc_free
> > 
> > There is no point in registering memory with the garbage collector
> > if it doesn't need to do its job. In fact, calling scm_gc_free in
> > a free function is wrong because it relies on Java finalization.
> > 
> > * libguile/random.c (scm_c_random_bignum): Replace matching calls of
> >   scm_gc_calloc and scm_gc_free.
> > * libguile/regex-posix.c (scm_make_regexp, regex_free): Avoid call
> >   of scm_gc_free in free function.
> > * test-suite/standalone/test-smob-mark.c (make_x, free_x): Avoid
> >   call of scm_gc_free in free function.
> > ---
> >  THANKS                                 | 1 +
> >  libguile/random.c                      | 8 ++------
> >  libguile/regex-posix.c                 | 6 +++---
> >  test-suite/standalone/test-smob-mark.c | 4 ++--
> >  4 files changed, 8 insertions(+), 11 deletions(-)
> > 
> > diff --git a/THANKS b/THANKS
> > index aa4877e95..786b65d1a 100644
> > --- a/THANKS
> > +++ b/THANKS
> > @@ -13,6 +13,7 @@ Contributors since the last release:
> >           Volker Grabsch
> >           Julian Graham
> >          Michael Gran
> > +          Jonas Hahnfeld
> >           Daniel Hartwig
> >               No Itisnt
> >             Neil Jerram
> > diff --git a/libguile/random.c b/libguile/random.c
> > index 63da7f5d6..ac400a9fd 100644
> > --- a/libguile/random.c
> > +++ b/libguile/random.c
> > @@ -324,9 +324,7 @@ scm_c_random_bignum (scm_t_rstate *state, SCM m)
> >    /* we know the result will be this big */
> >    mpz_realloc2 (SCM_I_BIG_MPZ (result), m_bits);
> >  
> > -  random_chunks =
> > -    (uint32_t *) scm_gc_calloc (num_chunks * sizeof (uint32_t),
> > -                                     "random bignum chunks");
> > +  random_chunks = (uint32_t *) scm_calloc (num_chunks * sizeof
> > (uint32_t));
> >  
> >    do
> >      {
> > @@ -363,9 +361,7 @@ scm_c_random_bignum (scm_t_rstate *state, SCM m)
> >        /* if result >= m, regenerate it (it is important to
> > regenerate
> >  	 all bits in order not to get a distorted distribution) */
> >      } while (mpz_cmp (SCM_I_BIG_MPZ (result), SCM_I_BIG_MPZ (m)) >=
> > 0);
> > -  scm_gc_free (random_chunks,
> > -               num_chunks * sizeof (uint32_t),
> > -               "random bignum chunks");
> > +  free (random_chunks);
> 
> From the manual (about scm_gc_calloc & friends):
> 
> ‘Memory blocks allocated this way may
> be released explicitly; however, this is not strictly needed, and we
> recommend _not_ calling ‘scm_gc_free’.  All memory allocated with
> ‘scm_gc_malloc’ or ‘scm_gc_malloc_pointerless’ is automatically
> reclaimed when the garbage collector no longer sees any live reference
> to it(1).’
> 
> As such, I'd recommend simply dropping the scm_gc_free
> (here and in other places), if the scm_gc_free was problematic
> because of Java finalization reasons.

We could, but what is the point of registering memory to the garbage
collector where we know it won't survive the function?

> 
> 
> >    return scm_i_normbig (result);
> >  }
> >  
> > diff --git a/libguile/regex-posix.c b/libguile/regex-posix.c
> > index a08da02db..36bb639e0 100644
> > --- a/libguile/regex-posix.c
> > +++ b/libguile/regex-posix.c
> > @@ -67,7 +67,7 @@ static size_t
> >  regex_free (SCM obj)
> >  {
> >    regfree (SCM_RGX (obj));
> > -  scm_gc_free (SCM_RGX (obj), sizeof(regex_t), "regex");
> > +  free (SCM_RGX (obj));
> >    return 0;
> >  }
> >  
> > @@ -164,7 +164,7 @@ SCM_DEFINE (scm_make_regexp, "make-regexp", 1, 0,
> > 1,
> >        flag = SCM_CDR (flag);
> >      }
> >  
> > -  rx = scm_gc_malloc_pointerless (sizeof (regex_t), "regex");
> > +  rx = scm_malloc (sizeof (regex_t));
> 
> If the regex why scm_gc_malloc_pointerless -> scm_malloc?
> Is rx not pointerless?

Not sure I understand the question. We don't know what contents libc
will write into regex_t. It could be pointers which would be bad for
the garbage collector.

> You coud simply ...
> 
> 
> > -      scm_gc_free (rx, sizeof(regex_t), "regex");
> > +      free (rx);
> 
> drop the scm_gc_free AFAIK.

No, I cannot as explained in the patch summary: If we use scm_gc_free
in a free function of a Smob, this relies on Java finalization because
the memory must not be reclaimed in the same cycle.

Jonas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 13:32     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
@ 2021-11-19 13:35       ` Maxime Devos
  2021-11-19 13:40         ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-11-19 13:48       ` Maxime Devos
  1 sibling, 1 reply; 29+ messages in thread
From: Maxime Devos @ 2021-11-19 13:35 UTC (permalink / raw)
  To: Jonas Hahnfeld, guile-devel

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

Jonas Hahnfeld schreef op vr 19-11-2021 om 14:32 [+0100]:
> > You coud simply ...
> > 
> > 
> > > -      scm_gc_free (rx, sizeof(regex_t), "regex");
> > > +      free (rx);
> > 
> > drop the scm_gc_free AFAIK.
> 
> No, I cannot as explained in the patch summary: If we use scm_gc_free
> in a free function of a Smob, this relies on Java finalization
> because
> the memory must not be reclaimed in the same cycle.

The suggestion was to remove scm_gc_free, and not introduce free.
I.e., don't free rx manually at all, let boehmgc decide:

 regex_free (SCM obj)
 {
   regfree (SCM_RGX (obj));
-  scm_gc_free (SCM_RGX (obj), sizeof(regex_t), "regex");
   return 0;
 }

Greetings,
Maxime

[-- Attachment #2: Type: text/html, Size: 2520 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 13:15   ` Maxime Devos
@ 2021-11-19 13:35     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-11-19 14:21       ` Maxime Devos
  0 siblings, 1 reply; 29+ messages in thread
From: Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library @ 2021-11-19 13:35 UTC (permalink / raw)
  To: Maxime Devos, guile-devel

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

Am Freitag, dem 19.11.2021 um 13:15 +0000 schrieb Maxime Devos:
> Jonas Hahnfeld schreef op do 15-07-2021 om 20:44 [+0200]:
> > +  SCM *smobs = scm_gc_malloc (sizeof(SCM) * SMOBS_COUNT, "smobs");
> > +
> >    int i;
> >    mark_call_count = 0;
> >    for (i = 0; i < SMOBS_COUNT; i++)
> > -    make_x ();
> > +    smobs[i] = make_x ();
> >    scm_gc ();
> 
> smobs doesn't need to be protected for the whole function call,
> until after the scm_gc() should be sufficient I think. 

That's what the patch does, no? For reference, the whole function
(after this patch) looks like:

SCM *smobs = scm_gc_malloc (sizeof(SCM) * SMOBS_COUNT, "smobs");

int i;
mark_call_count = 0;
for (i = 0; i < SMOBS_COUNT; i++)
  smobs[i] = make_x (); 
scm_gc (); 
if (mark_call_count < SMOBS_COUNT)
  {   
    fprintf (stderr, "FAIL: SMOB mark function called for each SMOB\n");
    exit (EXIT_FAILURE);
  }   
scm_remember_upto_here_1 (smobs);

While we could move the remember_upto_here immediately after the call
to scm_gc(), the current version ensures that the memory is still
available when the error is checked.

Jonas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 13:17   ` Maxime Devos
@ 2021-11-19 13:38     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  0 siblings, 0 replies; 29+ messages in thread
From: Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library @ 2021-11-19 13:38 UTC (permalink / raw)
  To: Maxime Devos, guile-devel

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

Am Freitag, dem 19.11.2021 um 13:17 +0000 schrieb Maxime Devos:
> Jonas Hahnfeld schreef op do 15-07-2021 om 20:44 [+0200]:
> > +  /* For guardians, we use unordered finalization `a la Java. */
> 
> Maybe add the reasons why this is only done when a guardian is created?
> E.g.,
> 
> /* Don't use unordered finalization when not using guardians,
>     because Java finalization prevents fast collection of chains of
> unreachable objects */

I think this is a misunderstanding: bdwgc still defaults to Java-style
finalization, my patches only make it possible to disable it while
maintaining a working Guile.

> Not 100% about the exact purpose of avoiding Java-style finalization,
> you might want to adjust the wording somewhat ...

As discussed in July, it's a possible optimization because it allows
the garbage collector to reclaim more memory at once. I'm not sure if
it's worth it in real applications, but making Guile ready seemed like
a good idea.

Jonas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 13:35       ` Maxime Devos
@ 2021-11-19 13:40         ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-11-19 13:44           ` Maxime Devos
  0 siblings, 1 reply; 29+ messages in thread
From: Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library @ 2021-11-19 13:40 UTC (permalink / raw)
  To: Maxime Devos, guile-devel

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

Am Freitag, dem 19.11.2021 um 13:35 +0000 schrieb Maxime Devos:
> Jonas Hahnfeld schreef op vr 19-11-2021 om 14:32 [+0100]:
> > > You coud simply ...
> > > 
> > > 
> > > > -      scm_gc_free (rx, sizeof(regex_t), "regex");
> > > > +      free (rx);
> > > 
> > > drop the scm_gc_free AFAIK.
> > 
> > No, I cannot as explained in the patch summary: If we use
> > scm_gc_free
> > in a free function of a Smob, this relies on Java finalization
> > because
> > the memory must not be reclaimed in the same cycle.
> 
> The suggestion was to remove scm_gc_free, and not introduce free.
> I.e., don't free rx manually at all, let boehmgc decide:
> 
>  regex_free (SCM obj)
>  {
>    regfree (SCM_RGX (obj));
> -  scm_gc_free (SCM_RGX (obj), sizeof(regex_t), "regex");
>    return 0;
>  }

This is dangerous because we still pass the memory to regfree, so it
must not be freed before.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 13:40         ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
@ 2021-11-19 13:44           ` Maxime Devos
  2021-11-19 13:53             ` Maxime Devos
  2021-11-19 13:53             ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  0 siblings, 2 replies; 29+ messages in thread
From: Maxime Devos @ 2021-11-19 13:44 UTC (permalink / raw)
  To: Jonas Hahnfeld, guile-devel

Jonas Hahnfeld schreef op vr 19-11-2021 om 14:40 [+0100]:
> Am Freitag, dem 19.11.2021 um 13:35 +0000 schrieb Maxime Devos:
> > Jonas Hahnfeld schreef op vr 19-11-2021 om 14:32 [+0100]:
> > > > You coud simply ...
> > > > 
> > > > 
> > > > > -      scm_gc_free (rx, sizeof(regex_t), "regex");
> > > > > +      free (rx);
> > > > 
> > > > drop the scm_gc_free AFAIK.
> > > 
> > > No, I cannot as explained in the patch summary: If we use
> > > scm_gc_free
> > > in a free function of a Smob, this relies on Java finalization
> > > because
> > > the memory must not be reclaimed in the same cycle.
> > 
> > The suggestion was to remove scm_gc_free, and not introduce free.
> > I.e., don't free rx manually at all, let boehmgc decide:
> > 
> >  regex_free (SCM obj)
> >  {
> >    regfree (SCM_RGX (obj));
> > -  scm_gc_free (SCM_RGX (obj), sizeof(regex_t), "regex");
> >    return 0;
> >  }
> 
> This is dangerous because we still pass the memory to regfree, so it
> must not be freed before.

How can removing a call to a free function introduce new use-after-free
bugs or double-free bugs? AFAIK, ignoring memory leak concerns (which
don't seem to apply here because of boehmgc), freeing less stuff cannot
introduce new bugs.

Greetings,
Maxime




^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 13:32     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-11-19 13:35       ` Maxime Devos
@ 2021-11-19 13:48       ` Maxime Devos
  2021-11-19 13:55         ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  1 sibling, 1 reply; 29+ messages in thread
From: Maxime Devos @ 2021-11-19 13:48 UTC (permalink / raw)
  To: Jonas Hahnfeld, guile-devel

Jonas Hahnfeld schreef op vr 19-11-2021 om 14:32 [+0100]:
> > > -  rx = scm_gc_malloc_pointerless (sizeof (regex_t), "regex");
> > > +  rx = scm_malloc (sizeof (regex_t));
> > 
> > If the regex why scm_gc_malloc_pointerless -> scm_malloc?
> > Is rx not pointerless?
> 
> Not sure I understand the question. We don't know what contents libc
> will write into regex_t. It could be pointers which would be bad for
> the garbage collector.

OK, if that's the case, seems like a bug in the original code, not
related to Java-style finalisation, so I would do that in a separate
patch.  Though libc probably allocates stuff with malloc and frees it
with free, and we call regfree to tell libc, so I think we should be
fine?

Greetings,
Maxime




^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 13:44           ` Maxime Devos
@ 2021-11-19 13:53             ` Maxime Devos
  2021-11-19 13:53             ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  1 sibling, 0 replies; 29+ messages in thread
From: Maxime Devos @ 2021-11-19 13:53 UTC (permalink / raw)
  To: Jonas Hahnfeld, guile-devel

Maxime Devos schreef op vr 19-11-2021 om 13:44 [+0000]:
> Jonas Hahnfeld schreef op vr 19-11-2021 om 14:40 [+0100]:
> > Am Freitag, dem 19.11.2021 um 13:35 +0000 schrieb Maxime Devos:
> > > Jonas Hahnfeld schreef op vr 19-11-2021 om 14:32 [+0100]:
> > > > > You coud simply ...
> > > > > 
> > > > > 
> > > > > > -      scm_gc_free (rx, sizeof(regex_t), "regex");
> > > > > > +      free (rx);
> > > > > 
> > > > > drop the scm_gc_free AFAIK.
> > > > 
> > > > No, I cannot as explained in the patch summary: If we use
> > > > scm_gc_free
> > > > in a free function of a Smob, this relies on Java finalization
> > > > because
> > > > the memory must not be reclaimed in the same cycle.
> > > 
> > > The suggestion was to remove scm_gc_free, and not introduce free.
> > > I.e., don't free rx manually at all, let boehmgc decide:
> > > 
> > >  regex_free (SCM obj)
> > >  {
> > >    regfree (SCM_RGX (obj));
> > > -  scm_gc_free (SCM_RGX (obj), sizeof(regex_t), "regex");
> > >    return 0;
> > >  }
> > 
> > This is dangerous because we still pass the memory to regfree, so
> > it
> > must not be freed before.
> 
> How can removing a call to a free function introduce new use-after-
> free
> bugs or double-free bugs? AFAIK, ignoring memory leak concerns (which
> don't seem to apply here because of boehmgc), freeing less stuff
> cannot
> introduce new bugs.

Unless scm_gc_free is called to act as scm_remember_upto_here,
such that the memory regex_t isn't freed until regfree has done what
it should?  But regfree is called with the regex_t, so
scm_remember_upto_here wouldn't be needed.

Unless it actually is because of how finalisation in boehmgc interacts
with how boehmgc keeps track of what can be collected and what not
(I don't know the details).


Greetings,
Maxime.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 13:44           ` Maxime Devos
  2021-11-19 13:53             ` Maxime Devos
@ 2021-11-19 13:53             ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-11-19 14:01               ` Maxime Devos
  1 sibling, 1 reply; 29+ messages in thread
From: Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library @ 2021-11-19 13:53 UTC (permalink / raw)
  To: Maxime Devos, guile-devel

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

Am Freitag, dem 19.11.2021 um 13:44 +0000 schrieb Maxime Devos:
> Jonas Hahnfeld schreef op vr 19-11-2021 om 14:40 [+0100]:
> > Am Freitag, dem 19.11.2021 um 13:35 +0000 schrieb Maxime Devos:
> > > Jonas Hahnfeld schreef op vr 19-11-2021 om 14:32 [+0100]:
> > > > > You coud simply ...
> > > > > 
> > > > > 
> > > > > > -      scm_gc_free (rx, sizeof(regex_t), "regex");
> > > > > > +      free (rx);
> > > > > 
> > > > > drop the scm_gc_free AFAIK.
> > > > 
> > > > No, I cannot as explained in the patch summary: If we use
> > > > scm_gc_free
> > > > in a free function of a Smob, this relies on Java finalization
> > > > because
> > > > the memory must not be reclaimed in the same cycle.
> > > 
> > > The suggestion was to remove scm_gc_free, and not introduce free.
> > > I.e., don't free rx manually at all, let boehmgc decide:
> > > 
> > >  regex_free (SCM obj)
> > >  {
> > >    regfree (SCM_RGX (obj));
> > > -  scm_gc_free (SCM_RGX (obj), sizeof(regex_t), "regex");
> > >    return 0;
> > >  }
> > 
> > This is dangerous because we still pass the memory to regfree, so it
> > must not be freed before.
> 
> How can removing a call to a free function introduce new use-after-free
> bugs or double-free bugs? AFAIK, ignoring memory leak concerns (which
> don't seem to apply here because of boehmgc), freeing less stuff cannot
> introduce new bugs.

It doesn't introduce a new bug, I'm just trying to explain that there
already is a bug / an implicit dependence on Java-style finalization.
Let me try to explain my understanding how it works, and why this is
bad:

scm_make_regexp currently uses scm_gc_malloc_pointerless to allocate
memory for regex_t. The pointer is stored in a newly created smob,
whose memory is also under the control of the garbage collector. In
regex_free, the regex_t is passed to regfree, which requires that the
memory is still there and not freed yet.
This "happens to work" with Java-style finalization because the smob
points to the regex_t and so the two are not collected in one go. This
assumption breaks in the more "greedy" mode where entire chains of
unreachable objects may be collected at once. Removing scm_gc_free
"solves" the immediate crash (the GC complaining that the memory has
already been freed), but regfree still attempts to read from the
memory.
The straight-forward solution is to statically tie the lifetime of
regex_t to that of the smob. Which we cannot do with GC, but simply
with malloc+free - as implemented in the patch.

Jonas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 13:48       ` Maxime Devos
@ 2021-11-19 13:55         ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-11-19 14:14           ` Maxime Devos
  0 siblings, 1 reply; 29+ messages in thread
From: Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library @ 2021-11-19 13:55 UTC (permalink / raw)
  To: Maxime Devos, guile-devel

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

Am Freitag, dem 19.11.2021 um 13:48 +0000 schrieb Maxime Devos:
> Jonas Hahnfeld schreef op vr 19-11-2021 om 14:32 [+0100]:
> > > > -  rx = scm_gc_malloc_pointerless (sizeof (regex_t), "regex");
> > > > +  rx = scm_malloc (sizeof (regex_t));
> > > 
> > > If the regex why scm_gc_malloc_pointerless -> scm_malloc?
> > > Is rx not pointerless?
> > 
> > Not sure I understand the question. We don't know what contents libc
> > will write into regex_t. It could be pointers which would be bad for
> > the garbage collector.
> 
> OK, if that's the case, seems like a bug in the original code, not
> related to Java-style finalisation, so I would do that in a separate
> patch.

Again, as replied in July to the same comment, it *is* a separate patch
for exactly this reason.

> Though libc probably allocates stuff with malloc and frees it
> with free, and we call regfree to tell libc, so I think we should be
> fine?

If memory lives until we call regfree, yes. See my other reply on why
this currently requires Java-style finalization to "work by accident".

Jonas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 13:53             ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
@ 2021-11-19 14:01               ` Maxime Devos
  0 siblings, 0 replies; 29+ messages in thread
From: Maxime Devos @ 2021-11-19 14:01 UTC (permalink / raw)
  To: Jonas Hahnfeld, guile-devel

Jonas Hahnfeld schreef op vr 19-11-2021 om 14:53 [+0100]:
> [...]
> The straight-forward solution is to statically tie the lifetime of
> regex_t to that of the smob. Which we cannot do with GC, but simply
> with malloc+free - as implemented in the patch.

OK, but for clarity I recommend adding a comment like

/* When not using Java-style finalisation, we must make sure
   the memory for the regex_t is only freed after regfree.
   To do so, use scm_malloc+free instead of scm_gc_free such
   that boehmgc will only free the regex_t when we ask it to
   in regex_free, and it won't automatically free the regex_t
   too early. */

Otherwise, it would be easy to think ‘Hmm, this code would be
simpler if we just use scm_gc_malloc and remove the unnecessary
free’ and accidentally introduce a bug ...

Greetings,
Maxime




^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 13:55         ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
@ 2021-11-19 14:14           ` Maxime Devos
  2021-11-19 14:52             ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  0 siblings, 1 reply; 29+ messages in thread
From: Maxime Devos @ 2021-11-19 14:14 UTC (permalink / raw)
  To: Jonas Hahnfeld, guile-devel

Jonas Hahnfeld schreef op vr 19-11-2021 om 14:55 [+0100]:
> Am Freitag, dem 19.11.2021 um 13:48 +0000 schrieb Maxime Devos:
> > Jonas Hahnfeld schreef op vr 19-11-2021 om 14:32 [+0100]:
> > > > > -  rx = scm_gc_malloc_pointerless (sizeof (regex_t),
> > > > > "regex");
> > > > > +  rx = scm_malloc (sizeof (regex_t));
> > > > 
> > > > If the regex why scm_gc_malloc_pointerless -> scm_malloc?
> > > > Is rx not pointerless?
> > > 
> > > Not sure I understand the question. We don't know what contents
> > > libc
> > > will write into regex_t. It could be pointers which would be bad
> > > for
> > > the garbage collector.
> > 
> > OK, if that's the case, seems like a bug in the original code, not
> > related to Java-style finalisation, so I would do that in a
> > separate
> > patch.

From your other responses, I now know it is actually related to (non-
)Java style finalisation, but my comment about ‘separate patch’ still
seems to apply:

> 
> Again, as replied in July to the same comment, it *is* a separate
> patch
> for exactly this reason.

More concretely, it is in the same patch as that modified
libguile/random.c.  The patch to libguile/random.c doesn't seem to
be for non-Java finalization reasons. Going by the commit message,
the only possible reason I could find is:

‘There is no point in registering memory with the garbage collector
if it doesn't need to do its job’

But I don't see any ‘registering memory’, only replacing
scm_gc_calloc+scm_gc_free by scm_calloc+free, and without any
finalisation in sight. Unless you mean with ‘registering memory’
the "random bignum chunks" argument. But that still seems unrelated
to non-Java finalization.

Greetings,
Maxime




^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 13:35     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
@ 2021-11-19 14:21       ` Maxime Devos
  2021-11-19 15:32         ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  0 siblings, 1 reply; 29+ messages in thread
From: Maxime Devos @ 2021-11-19 14:21 UTC (permalink / raw)
  To: Jonas Hahnfeld, guile-devel

Jonas Hahnfeld schreef op vr 19-11-2021 om 14:35 [+0100]:
> Am Freitag, dem 19.11.2021 um 13:15 +0000 schrieb Maxime Devos:
> > Jonas Hahnfeld schreef op do 15-07-2021 om 20:44 [+0200]:
> > > +  SCM *smobs = scm_gc_malloc (sizeof(SCM) * SMOBS_COUNT,
> > > "smobs");
> > > +
> > >    int i;
> > >    mark_call_count = 0;
> > >    for (i = 0; i < SMOBS_COUNT; i++)
> > > -    make_x ();
> > > +    smobs[i] = make_x ();
> > >    scm_gc ();
> > 
> > smobs doesn't need to be protected for the whole function call,
> > until after the scm_gc() should be sufficient I think. 
> 
> That's what the patch does, no? For reference, the whole function
> (after this patch) looks like:
> 
> SCM *smobs = scm_gc_malloc (sizeof(SCM) * SMOBS_COUNT, "smobs");
> 
> int i;
> mark_call_count = 0;
> for (i = 0; i < SMOBS_COUNT; i++)
>   smobs[i] = make_x (); 
> scm_gc (); 
> if (mark_call_count < SMOBS_COUNT)
>   {   
>     fprintf (stderr, "FAIL: SMOB mark function called for each
> SMOB\n");
>     exit (EXIT_FAILURE);
>   }   
> scm_remember_upto_here_1 (smobs);
> 
> While we could move the remember_upto_here immediately after the call
> to scm_gc(), the current version ensures that the memory is still
> available when the error is checked.

The error checking code (I'm thinking of the if (mark_call_count ...)
fprintf(...) exit(...) here) isn't using 'smobs', so the error checking
code doesn't need the memory to be available AFAIK.

Greetings,
Maxime Devos.




^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 14:14           ` Maxime Devos
@ 2021-11-19 14:52             ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  2021-11-19 18:48               ` Maxime Devos
  0 siblings, 1 reply; 29+ messages in thread
From: Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library @ 2021-11-19 14:52 UTC (permalink / raw)
  To: Maxime Devos, guile-devel

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

Am Freitag, dem 19.11.2021 um 14:14 +0000 schrieb Maxime Devos:
> Jonas Hahnfeld schreef op vr 19-11-2021 om 14:55 [+0100]:
> > Am Freitag, dem 19.11.2021 um 13:48 +0000 schrieb Maxime Devos:
> > > Jonas Hahnfeld schreef op vr 19-11-2021 om 14:32 [+0100]:
> > > > > > -  rx = scm_gc_malloc_pointerless (sizeof (regex_t),
> > > > > > "regex");
> > > > > > +  rx = scm_malloc (sizeof (regex_t));
> > > > > 
> > > > > If the regex why scm_gc_malloc_pointerless -> scm_malloc?
> > > > > Is rx not pointerless?
> > > > 
> > > > Not sure I understand the question. We don't know what contents
> > > > libc
> > > > will write into regex_t. It could be pointers which would be bad
> > > > for
> > > > the garbage collector.
> > > 
> > > OK, if that's the case, seems like a bug in the original code, not
> > > related to Java-style finalisation, so I would do that in a
> > > separate
> > > patch.
> 
> From your other responses, I now know it is actually related to (non-
> )Java style finalisation, but my comment about ‘separate patch’ still
> seems to apply:
> 
> > 
> > Again, as replied in July to the same comment, it *is* a separate
> > patch for exactly this reason.
> 
> More concretely, it is in the same patch as that modified
> libguile/random.c.  The patch to libguile/random.c doesn't seem to
> be for non-Java finalization reasons. Going by the commit message,
> the only possible reason I could find is:
> 
> ‘There is no point in registering memory with the garbage collector
> if it doesn't need to do its job’
> 
> But I don't see any ‘registering memory’, only replacing
> scm_gc_calloc+scm_gc_free by scm_calloc+free, and without any
> finalisation in sight. Unless you mean with ‘registering memory’
> the "random bignum chunks" argument. But that still seems unrelated
> to non-Java finalization.

Any memory allocation through gc implicitly registers the memory. Both
changes are unrelated to finalization, they are there to avoid this
unnecessary registration. My previous replies only tried to clarify why
any other solution is worse.

Another question: Do you actually have permission to apply my patches?
You already reviewed my patches in July, but as they weren't applied
back then, does this mean we need somebody else to actually get them
in?

Jonas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 14:21       ` Maxime Devos
@ 2021-11-19 15:32         ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  0 siblings, 0 replies; 29+ messages in thread
From: Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library @ 2021-11-19 15:32 UTC (permalink / raw)
  To: Maxime Devos, guile-devel

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

Am Freitag, dem 19.11.2021 um 14:21 +0000 schrieb Maxime Devos:
> Jonas Hahnfeld schreef op vr 19-11-2021 om 14:35 [+0100]:
> > Am Freitag, dem 19.11.2021 um 13:15 +0000 schrieb Maxime Devos:
> > > Jonas Hahnfeld schreef op do 15-07-2021 om 20:44 [+0200]:
> > > > +  SCM *smobs = scm_gc_malloc (sizeof(SCM) * SMOBS_COUNT,
> > > > "smobs");
> > > > +
> > > >    int i;
> > > >    mark_call_count = 0;
> > > >    for (i = 0; i < SMOBS_COUNT; i++)
> > > > -    make_x ();
> > > > +    smobs[i] = make_x ();
> > > >    scm_gc ();
> > > 
> > > smobs doesn't need to be protected for the whole function call,
> > > until after the scm_gc() should be sufficient I think. 
> > 
> > That's what the patch does, no? For reference, the whole function
> > (after this patch) looks like:
> > 
> > SCM *smobs = scm_gc_malloc (sizeof(SCM) * SMOBS_COUNT, "smobs");
> > 
> > int i;
> > mark_call_count = 0;
> > for (i = 0; i < SMOBS_COUNT; i++)
> >   smobs[i] = make_x (); 
> > scm_gc (); 
> > if (mark_call_count < SMOBS_COUNT)
> >   {   
> >     fprintf (stderr, "FAIL: SMOB mark function called for each
> > SMOB\n");
> >     exit (EXIT_FAILURE);
> >   }   
> > scm_remember_upto_here_1 (smobs);
> > 
> > While we could move the remember_upto_here immediately after the call
> > to scm_gc(), the current version ensures that the memory is still
> > available when the error is checked.
> 
> The error checking code (I'm thinking of the if (mark_call_count ...)
> fprintf(...) exit(...) here) isn't using 'smobs', so the error checking
> code doesn't need the memory to be available AFAIK.

No, but if somebody ever wants to debug the test, it will be less
surprising if things are still there, or at least not risking that they
are gone at a moment's notice. So, what's the gain of moving it?

Jonas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 14:52             ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
@ 2021-11-19 18:48               ` Maxime Devos
  2021-11-19 19:01                 ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  0 siblings, 1 reply; 29+ messages in thread
From: Maxime Devos @ 2021-11-19 18:48 UTC (permalink / raw)
  To: Jonas Hahnfeld, guile-devel

Jonas Hahnfeld schreef op vr 19-11-2021 om 15:52 [+0100]:
> Am Freitag, dem 19.11.2021 um 14:14 +0000 schrieb Maxime Devos:
> > [...]
> > 
> > From your other responses, I now know it is actually related to
> > (non-
> > )Java style finalisation, but my comment about ‘separate patch’
> > still
> > seems to apply:
> > 
> > > 
> > > Again, as replied in July to the same comment, it *is* a separate
> > > patch for exactly this reason.
> > 
> > More concretely, it is in the same patch as that modified
> > libguile/random.c.  The patch to libguile/random.c doesn't seem to
> > be for non-Java finalization reasons. Going by the commit message,
> > the only possible reason I could find is:
> > 
> > ‘There is no point in registering memory with the garbage collector
> > if it doesn't need to do its job’
> > 
> > But I don't see any ‘registering memory’, only replacing
> > scm_gc_calloc+scm_gc_free by scm_calloc+free, and without any
> > finalisation in sight. Unless you mean with ‘registering memory’
> > the "random bignum chunks" argument. But that still seems unrelated
> > to non-Java finalization.
> 
> Any memory allocation through gc implicitly registers the memory.

I don't mean what you mean with ‘registering memory’. I don't
see that phrase anywhere at <https://www.hboehm.info/gc/#details>
or <https://www.hboehm.info/gc/faq.html>.  I only know about
registering finalisers, but not about registering memory.

Also, I'm not sure what you are trying to say here and in the following
paragraph.  Is this some kind of argument for why the change to
libguile/random.c should be in the same patch, or general explanation,
...?

> Both
> changes are unrelated to finalization, they are there to avoid this
> unnecessary registration.
Thanks for the clarification, though I have no idea what ‘registration’
is ...
>  My previous replies only tried to clarify why
> any other solution is worse.

... but what problem and what replies are you referring to here?
I haven't seen any e-mails explaining GC problems in libguile/random.c.
I only have seen replies about non-Java style finalisation, which
do not apply to libguile/random.c (no objects but the stack have a
reference to random_chunks anywhere and libguile/random.c is not
playing with finalizers).

> Another question: Do you actually have permission to apply my
> patches?
> You already reviewed my patches in July, but as they weren't applied
> back then, does this mean we need somebody else to actually get them
> in?

No, I don't have commit access. I noted in July
(<https://lists.gnu.org/archive/html/guile-devel/2021-07/msg00002.html>)
that I'm not a guile dev.

Greetings,
Maxime.




^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-11-19 18:48               ` Maxime Devos
@ 2021-11-19 19:01                 ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
  0 siblings, 0 replies; 29+ messages in thread
From: Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library @ 2021-11-19 19:01 UTC (permalink / raw)
  To: Maxime Devos, guile-devel

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

Am Freitag, dem 19.11.2021 um 18:48 +0000 schrieb Maxime Devos:
> Jonas Hahnfeld schreef op vr 19-11-2021 om 15:52 [+0100]:
> > Am Freitag, dem 19.11.2021 um 14:14 +0000 schrieb Maxime Devos:
> > > [...]
> > > 
> > > From your other responses, I now know it is actually related to
> > > (non-
> > > )Java style finalisation, but my comment about ‘separate patch’
> > > still
> > > seems to apply:
> > > 
> > > > 
> > > > Again, as replied in July to the same comment, it *is* a separate
> > > > patch for exactly this reason.
> > > 
> > > More concretely, it is in the same patch as that modified
> > > libguile/random.c.  The patch to libguile/random.c doesn't seem to
> > > be for non-Java finalization reasons. Going by the commit message,
> > > the only possible reason I could find is:
> > > 
> > > ‘There is no point in registering memory with the garbage collector
> > > if it doesn't need to do its job’
> > > 
> > > But I don't see any ‘registering memory’, only replacing
> > > scm_gc_calloc+scm_gc_free by scm_calloc+free, and without any
> > > finalisation in sight. Unless you mean with ‘registering memory’
> > > the "random bignum chunks" argument. But that still seems unrelated
> > > to non-Java finalization.
> > 
> > Any memory allocation through gc implicitly registers the memory.
> 
> I don't mean what you mean with ‘registering memory’. I don't
> see that phrase anywhere at <https://www.hboehm.info/gc/#details>
> or <https://www.hboehm.info/gc/faq.html>.  I only know about
> registering finalisers, but not about registering memory.

Maybe it's not an official term; I call it "registration" because the
garbage collector has to keep track of all memory segments it handed
out, so it "registers" the allocation in a data structure somewhere.

> Also, I'm not sure what you are trying to say here and in the following
> paragraph.  Is this some kind of argument for why the change to
> libguile/random.c should be in the same patch, or general explanation,
> ...?

Both changes address the same issue of removing matching calls to
scm_gc_alloc+scm_gc_free. That's why I think it's one logical change,
even though one is actually a problem when disabling Java-style
finalization - it makes sense even without the following patch.

> 
> > Both changes are unrelated to finalization, they are there to avoid this
> > unnecessary registration.
> Thanks for the clarification, though I have no idea what ‘registration’
> is ...
> >  My previous replies only tried to clarify why
> > any other solution is worse.
> 
> ... but what problem and what replies are you referring to here?
> I haven't seen any e-mails explaining GC problems in libguile/random.c.
> I only have seen replies about non-Java style finalisation, which
> do not apply to libguile/random.c (no objects but the stack have a
> reference to random_chunks anywhere and libguile/random.c is not
> playing with finalizers).

Yes, the one in libguile/random.c is just unnecessary, probably not
actually a problem.

Jonas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: GC + Java finalization
  2021-07-15 18:44 ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
                     ` (3 preceding siblings ...)
  2021-11-19 13:17   ` Maxime Devos
@ 2021-11-20  9:19   ` Maxime Devos
  4 siblings, 0 replies; 29+ messages in thread
From: Maxime Devos @ 2021-11-20  9:19 UTC (permalink / raw)
  To: Jonas Hahnfeld, guile-devel

Jonas Hahnfeld schreef op do 15-07-2021 om 20:44 [+0200]:
> diff --git a/libguile/random.c b/libguile/random.c
> index 63da7f5d6..ac400a9fd 100644
> --- a/libguile/random.c
> +++ b/libguile/random.c
> @@ -324,9 +324,7 @@ scm_c_random_bignum (scm_t_rstate *state, SCM m)
>    /* we know the result will be this big */
>    mpz_realloc2 (SCM_I_BIG_MPZ (result), m_bits);
>  
> -  random_chunks =
> -    (uint32_t *) scm_gc_calloc (num_chunks * sizeof (uint32_t),
> -                                     "random bignum chunks");
> +  random_chunks = (uint32_t *) scm_calloc (num_chunks * sizeof
> (uint32_t));
>  
>    do
>      {
> @@ -363,9 +361,7 @@ scm_c_random_bignum (scm_t_rstate *state, SCM m)
>        /* if result >= m, regenerate it (it is important to
> regenerate
>   all bits in order not to get a distorted distribution) */
>      } while (mpz_cmp (SCM_I_BIG_MPZ (result), SCM_I_BIG_MPZ (m)) >=
> 0);
> -  scm_gc_free (random_chunks,
> -               num_chunks * sizeof (uint32_t),
> -               "random bignum chunks");
> +  free (random_chunks);
>    return scm_i_normbig (result);
>  }

As I understand it, the idea of this change is to avoid boehmgc having
to track the memory random_chunks (‘memory registration’). However,
in-between the scm_calloc and free, mpz_import is called. Looking at
libguile/mini-gmp.c, this causes gmp_allocate_func to be called. This
variable is set by mp_set_memory_functions, which is called by
scm_init_number with the allocation function
custom_gmp_malloc/custom_gmp_realloc, which uses
scm_gc_malloc_pointerless/scm_gc_realloc.

Note that that these functions signal an error in case of out-of-memory
(at least, that's what 6.19.2 Memory Blocks states). As such, in the
following situation a memory leak can happen after the proposed patch:

(catch 'out-of-memory
  (lambda ()
    ;; have enough memory to allocate random_chunks,
    ;; but not enough for mpz_import
    (random some-large-number))
  (lambda _
    ;; random_chunks won't ever be freed! 
    (pk 'oops-not-enough-memory _)))

At least Artanis tries to behave somewhat nicely in case of OOM
(https://git.savannah.gnu.org/cgit/artanis.git/tree/artanis/server/ragnarok.scm#n659),
so I'd prefer to keep using scm_gc_calloc (+ scm_gc_free) instead of
scm_calloc + free.

Greetings,
Maxime.




^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2021-11-20  9:19 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-03 12:05 GC + Java finalization Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
2021-07-03 17:14 ` Maxime Devos
2021-07-03 17:26   ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
2021-07-03 18:49     ` Maxime Devos
2021-07-03 18:54     ` Maxime Devos
2021-07-15 18:44 ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
2021-10-10 16:22   ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
2021-11-19 12:18     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
2021-11-19 13:13   ` Maxime Devos
2021-11-19 13:32     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
2021-11-19 13:35       ` Maxime Devos
2021-11-19 13:40         ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
2021-11-19 13:44           ` Maxime Devos
2021-11-19 13:53             ` Maxime Devos
2021-11-19 13:53             ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
2021-11-19 14:01               ` Maxime Devos
2021-11-19 13:48       ` Maxime Devos
2021-11-19 13:55         ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
2021-11-19 14:14           ` Maxime Devos
2021-11-19 14:52             ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
2021-11-19 18:48               ` Maxime Devos
2021-11-19 19:01                 ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
2021-11-19 13:15   ` Maxime Devos
2021-11-19 13:35     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
2021-11-19 14:21       ` Maxime Devos
2021-11-19 15:32         ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
2021-11-19 13:17   ` Maxime Devos
2021-11-19 13:38     ` Jonas Hahnfeld via Developers list for Guile, the GNU extensibility library
2021-11-20  9:19   ` Maxime Devos

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).