Re: GC + Java finalization

[Jonas Hahnfeld via "Developers list for Guile, the GNU extensibility library" <guile-devel@gnu.org>]

[-- 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 --]