From c9910c604279f438728cd268272e1839cbc53835 Mon Sep 17 00:00:00 2001 From: Andy Wingo Date: Mon, 13 Mar 2017 15:47:51 +0100 Subject: [PATCH] Fix finalizer resuscitation causing excessive GC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * libguile/finalizers.c (async_gc_finalizer): (scm_i_register_async_gc_callback): Replace "weak gc callback" mechanism with "async gc callback" mechanism. Very similar but the new API is designed to be called a bounded number of times, to avoid running afoul of libgc heuristics. * libguile/weak-list.h: New internal header. * libguile/Makefile.am (noinst_HEADERS): Add weak-list.h. * libguile/weak-set.c (vacuum_all_weak_sets): (scm_c_make_weak_set, scm_init_weak_set): * libguile/weak-table.c (vacuum_all_weak_tables): (scm_c_make_weak_table, scm_init_weak_table): Arrange to vacuum all weak sets from a single async GC callback, and likewise for weak tables. Thanks to Ludovic Courtès for tracking this bug down! --- libguile/Makefile.am | 4 ++- libguile/finalizers.c | 59 ++++++++++++++-------------------- libguile/finalizers.h | 8 ++--- libguile/weak-list.h | 73 +++++++++++++++++++++++++++++++++++++++++++ libguile/weak-set.c | 18 ++++++++++- libguile/weak-table.c | 18 ++++++++++- 6 files changed, 137 insertions(+), 43 deletions(-) create mode 100644 libguile/weak-list.h diff --git a/libguile/Makefile.am b/libguile/Makefile.am index 07466069f..142e739fb 100644 --- a/libguile/Makefile.am +++ b/libguile/Makefile.am @@ -507,7 +507,9 @@ noinst_HEADERS = conv-integer.i.c conv-uinteger.i.c \ atomics-internal.h \ cache-internal.h \ posix-w32.h \ - private-options.h ports-internal.h + private-options.h \ + ports-internal.h \ + weak-list.h # vm instructions noinst_HEADERS += vm-engine.c diff --git a/libguile/finalizers.c b/libguile/finalizers.c index 9b9075830..c5d69e8e3 100644 --- a/libguile/finalizers.c +++ b/libguile/finalizers.c @@ -296,59 +296,46 @@ scm_i_finalizer_pre_fork (void) -static void* -weak_pointer_ref (void *weak_pointer) -{ - return *(void **) weak_pointer; -} - static void -weak_gc_finalizer (void *ptr, void *data) +async_gc_finalizer (void *ptr, void *data) { - void **weak = ptr; - void *val; - void (*callback) (SCM) = weak[1]; + void **obj = ptr; + void (*callback) (void) = obj[0]; - val = GC_call_with_alloc_lock (weak_pointer_ref, &weak[0]); + callback (); - if (!val) - return; - - callback (SCM_PACK_POINTER (val)); - - scm_i_set_finalizer (ptr, weak_gc_finalizer, data); + scm_i_set_finalizer (ptr, async_gc_finalizer, data); } -/* CALLBACK will be called on OBJ, as long as OBJ is accessible. It - will be called from a finalizer, which may be from an async or from +/* Arrange to call CALLBACK asynchronously after each GC. The callback + will be invoked from a finalizer, which may be from an async or from another thread. - As an implementation detail, the way this works is that we allocate - a fresh pointer-less object holding two words. We know that this + As an implementation detail, the way this works is that we allocate a + fresh object and put the callback in the object. We know that this object should get collected the next time GC is run, so we attach a - finalizer to it so that we get a callback after GC happens. + finalizer to it to trigger the callback. - The first word of the object holds a weak reference to OBJ, and the - second holds the callback pointer. When the callback is called, we - check if the weak reference on OBJ still holds. If it doesn't hold, - then OBJ is no longer accessible, and we're done. Otherwise we call - the callback and re-register a finalizer for our two-word GC object, - effectively resuscitating the object so that we will get a callback - on the next GC. + Once the callback runs, we re-attach a finalizer to that fresh object + to prepare for the next GC, and the process repeats indefinitely. We could use the scm_after_gc_hook, but using a finalizer has the advantage of potentially running in another thread, decreasing pause - time. */ + time. + + Note that libgc currently has a heuristic that adding 500 finalizable + objects will cause GC to collect rather than expand the heap, + drastically reducing performance on workloads that actually need to + expand the heap. Therefore scm_i_register_async_gc_callback is + inappropriate for using on unbounded numbers of callbacks. */ void -scm_i_register_weak_gc_callback (SCM obj, void (*callback) (SCM)) +scm_i_register_async_gc_callback (void (*callback) (void)) { - void **weak = GC_MALLOC_ATOMIC (sizeof (void*) * 2); + void **obj = GC_MALLOC_ATOMIC (sizeof (void*)); - weak[0] = SCM_UNPACK_POINTER (obj); - weak[1] = (void*)callback; - GC_GENERAL_REGISTER_DISAPPEARING_LINK (weak, SCM2PTR (obj)); + obj[0] = (void*)callback; - scm_i_set_finalizer (weak, weak_gc_finalizer, NULL); + scm_i_set_finalizer (obj, async_gc_finalizer, NULL); } diff --git a/libguile/finalizers.h b/libguile/finalizers.h index d01d1b734..27b2cbf82 100644 --- a/libguile/finalizers.h +++ b/libguile/finalizers.h @@ -36,10 +36,10 @@ SCM_INTERNAL void scm_i_add_resuscitator (void *obj, scm_t_finalizer_proc, SCM_INTERNAL void scm_i_finalizer_pre_fork (void); -/* CALLBACK will be called on OBJ after each garbage collection, as long - as OBJ is accessible. It will be called from a finalizer, which may - be from an async or from another thread. */ -SCM_INTERNAL void scm_i_register_weak_gc_callback (SCM obj, void (*callback) (SCM)); +/* CALLBACK will be called after each garbage collection. It will be + called from a finalizer, which may be from an async or from another + thread. */ +SCM_INTERNAL void scm_i_register_async_gc_callback (void (*callback) (void)); SCM_API int scm_set_automatic_finalization_enabled (int enabled_p); SCM_API int scm_run_finalizers (void); diff --git a/libguile/weak-list.h b/libguile/weak-list.h new file mode 100644 index 000000000..989cb7f0a --- /dev/null +++ b/libguile/weak-list.h @@ -0,0 +1,73 @@ +/* classes: h_files */ + +#ifndef SCM_WEAK_LIST_H +#define SCM_WEAK_LIST_H + +/* Copyright (C) 2016 Free Software Foundation, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public License + * as published by the Free Software Foundation; either version 3 of + * the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301 USA + */ + + + +#include "libguile/__scm.h" +#include "libguile/weak-vector.h" + + + +static inline SCM +scm_i_weak_cons (SCM car, SCM cdr) +{ + return scm_cons (scm_c_make_weak_vector (1, car), cdr); +} + +static inline SCM +scm_i_weak_car (SCM pair) +{ + return scm_c_weak_vector_ref (scm_car (pair), 0); +} + +static inline void +scm_i_visit_weak_list (SCM *list_loc, void (*visit) (SCM)) +{ + SCM in = *list_loc, out = SCM_EOL; + + while (scm_is_pair (in)) + { + SCM car = scm_i_weak_car (in); + SCM cdr = scm_cdr (in); + + if (!scm_is_eq (car, SCM_BOOL_F)) + { + scm_set_cdr_x (in, out); + out = in; + visit (car); + } + + in = cdr; + } + + *list_loc = out; +} + + +#endif /* SCM_WEAK_LIST_H */ + +/* + Local Variables: + c-file-style: "gnu" + End: +*/ diff --git a/libguile/weak-set.c b/libguile/weak-set.c index d2e4744bf..1576e20b0 100644 --- a/libguile/weak-set.c +++ b/libguile/weak-set.c @@ -31,6 +31,7 @@ #include "libguile/bdw-gc.h" #include "libguile/validate.h" +#include "libguile/weak-list.h" #include "libguile/weak-set.h" @@ -698,6 +699,17 @@ do_vacuum_weak_set (SCM set) scm_i_pthread_mutex_unlock (&s->lock); } +static scm_i_pthread_mutex_t all_weak_sets_lock = SCM_I_PTHREAD_MUTEX_INITIALIZER; +static SCM all_weak_sets = SCM_EOL; + +static void +vacuum_all_weak_sets (void) +{ + scm_i_pthread_mutex_lock (&all_weak_sets_lock); + scm_i_visit_weak_list (&all_weak_sets, do_vacuum_weak_set); + scm_i_pthread_mutex_unlock (&all_weak_sets_lock); +} + SCM scm_c_make_weak_set (unsigned long k) { @@ -705,7 +717,9 @@ scm_c_make_weak_set (unsigned long k) ret = make_weak_set (k); - scm_i_register_weak_gc_callback (ret, do_vacuum_weak_set); + scm_i_pthread_mutex_lock (&all_weak_sets_lock); + all_weak_sets = scm_i_weak_cons (ret, all_weak_sets); + scm_i_pthread_mutex_unlock (&all_weak_sets_lock); return ret; } @@ -883,6 +897,8 @@ void scm_init_weak_set () { #include "libguile/weak-set.x" + + scm_i_register_async_gc_callback (vacuum_all_weak_sets); } /* diff --git a/libguile/weak-table.c b/libguile/weak-table.c index 1bb513b17..599c4cf0e 100644 --- a/libguile/weak-table.c +++ b/libguile/weak-table.c @@ -33,6 +33,7 @@ #include "libguile/ports.h" #include "libguile/validate.h" +#include "libguile/weak-list.h" #include "libguile/weak-table.h" @@ -832,6 +833,17 @@ do_vacuum_weak_table (SCM table) return; } +static scm_i_pthread_mutex_t all_weak_tables_lock = SCM_I_PTHREAD_MUTEX_INITIALIZER; +static SCM all_weak_tables = SCM_EOL; + +static void +vacuum_all_weak_tables (void) +{ + scm_i_pthread_mutex_lock (&all_weak_tables_lock); + scm_i_visit_weak_list (&all_weak_tables, do_vacuum_weak_table); + scm_i_pthread_mutex_unlock (&all_weak_tables_lock); +} + SCM scm_c_make_weak_table (unsigned long k, scm_t_weak_table_kind kind) { @@ -839,7 +851,9 @@ scm_c_make_weak_table (unsigned long k, scm_t_weak_table_kind kind) ret = make_weak_table (k, kind); - scm_i_register_weak_gc_callback (ret, do_vacuum_weak_table); + scm_i_pthread_mutex_lock (&all_weak_tables_lock); + all_weak_tables = scm_i_weak_cons (ret, all_weak_tables); + scm_i_pthread_mutex_unlock (&all_weak_tables_lock); return ret; } @@ -1155,6 +1169,8 @@ void scm_init_weak_table () { #include "libguile/weak-table.x" + + scm_i_register_async_gc_callback (vacuum_all_weak_tables); } /*