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
From c28444893ab14bd35381c7b60545fd248cc7d877 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld <hah...@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
From 7127986755f4500c0b000f769009d1240d163468 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld <hah...@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
From 02204f9a57e6608a23db6944f9176059966f9fd6 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld <hah...@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
signature.asc
Description: This is a digitally signed message part