Re: Fluids

[ludo@gnu.org (Ludovic Courtès)]

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

Hello,

ludo@gnu.org (Ludovic Courtès) writes:

> Andy Wingo <wingo@pobox.com> writes:
>
>> But you can't / shouldn't make a new fluid every time you enter a
>> `catch', because currently fluids are never garbage collected! We really
>> need to fix this. I think it's a 1.9 regression.
>
> Indeed.  We should use a weak vector or some such instead of the current
> scm_gc_malloc’d array.

Just to clarify: fluids themselves *are* GC’d, but fluid numbers aren’t
recycled so ALLOCATED_FLUIDS grows endlessly (1 byte per fluid).

The working patch below allows fluid numbers to be recycled but it’s
inefficient.  Needs more thought.

Thanks,
Ludo’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: the patch --]
[-- Type: text/x-patch, Size: 3928 bytes --]

diff --git a/libguile/fluids.c b/libguile/fluids.c
index d493053..157737a 100644
--- a/libguile/fluids.c
+++ b/libguile/fluids.c
@@ -34,6 +34,7 @@
 #include "libguile/deprecation.h"
 #include "libguile/lang.h"
 #include "libguile/validate.h"
+#include "libguile/bdw-gc.h"
 
 #define FLUID_GROW 20
 
@@ -63,7 +64,7 @@ static scm_i_pthread_mutex_t fluid_admin_mutex = SCM_I_PTHREAD_MUTEX_INITIALIZER
  */
 static size_t allocated_fluids_len = 0;
 static size_t allocated_fluids_num = 0;
-static char *allocated_fluids = NULL;
+static void **allocated_fluids = NULL;
 
 #define IS_FLUID(x)         SCM_I_FLUID_P (x)
 #define FLUID_NUM(x)        SCM_I_FLUID_NUM (x)
@@ -120,45 +121,53 @@ scm_i_dynamic_state_print (SCM exp, SCM port, scm_print_state *pstate SCM_UNUSED
   scm_putc ('>', port);
 }
 
-static size_t
-next_fluid_num ()
+static SCM
+new_fluid ()
 {
   size_t n;
+  SCM fluid;
+
+  /* Fluids are cells: the first word is the type tag; the second word is the
+     fluid number.  */
+  fluid = PTR2SCM (scm_gc_malloc_pointerless (sizeof (scm_t_cell), "fluid"));
+  SCM_SET_CELL_TYPE (fluid, scm_tc7_fluid);
+
+  /* Look for a fluid number.  */
 
   scm_dynwind_begin (0);
   scm_i_dynwind_pthread_mutex_lock (&fluid_admin_mutex);
 
-  if ((allocated_fluids_len > 0) &&
-      (allocated_fluids_num == allocated_fluids_len))
-    {
-      /* All fluid numbers are in use.  Run a GC to try to free some
-	 up.
-      */
-      scm_gc ();
-    }
-
   if (allocated_fluids_num < allocated_fluids_len)
+    n = allocated_fluids_num;
+  else
     {
+      /* All fluid numbers are in use.  Run a GC and look for a number recently
+	 freed.  */
+      scm_i_gc ("fluids");
+
       for (n = 0; n < allocated_fluids_len; n++)
-	if (allocated_fluids[n] == 0)
-	  break;
+	if (allocated_fluids[n] == NULL)
+	  {
+	    allocated_fluids_num--;
+	    break;
+	  }
     }
-  else
+
+  if (n >= allocated_fluids_len)
     {
       /* Grow the vector of allocated fluids.  */
-      /* FIXME: Since we use `scm_malloc ()', ALLOCATED_FLUIDS is scanned by
-	 the GC; therefore, all fluids remain reachable for the entire
-	 program lifetime.  Hopefully this is not a problem in practice.  */
-      char *new_allocated_fluids =
-	scm_gc_malloc (allocated_fluids_len + FLUID_GROW,
-		       "allocated fluids");
+      void **new_allocated_fluids =
+	scm_gc_malloc_pointerless ((allocated_fluids_len + FLUID_GROW)
+				   * sizeof (*allocated_fluids),
+				   "allocated fluids");
 
       /* Copy over old values and initialize rest.  GC can not run
 	 during these two operations since there is no safe point in
-	 them.
-      */
-      memcpy (new_allocated_fluids, allocated_fluids, allocated_fluids_len);
-      memset (new_allocated_fluids + allocated_fluids_len, 0, FLUID_GROW);
+	 them.  */
+      memcpy (new_allocated_fluids, allocated_fluids,
+	      allocated_fluids_len * sizeof (*allocated_fluids));
+      memset (new_allocated_fluids + allocated_fluids_len, 0,
+	      FLUID_GROW * sizeof (*allocated_fluids));
       n = allocated_fluids_len;
 
       /* Update the vector of allocated fluids.  Dynamic states will
@@ -167,12 +176,16 @@ next_fluid_num ()
       allocated_fluids = new_allocated_fluids;
       allocated_fluids_len += FLUID_GROW;
     }
-  
+
   allocated_fluids_num += 1;
-  allocated_fluids[n] = 1;
-  
+  allocated_fluids[n] = SCM2PTR (fluid);
+  SCM_SET_CELL_WORD_1 (fluid, (scm_t_bits) n);
+
+  GC_GENERAL_REGISTER_DISAPPEARING_LINK (&allocated_fluids[n],
+					 SCM2PTR (fluid));
+
   scm_dynwind_end ();
-  return n;
+  return fluid;
 }
 
 SCM_DEFINE (scm_make_fluid, "make-fluid", 0, 0, 0, 
@@ -186,7 +199,7 @@ SCM_DEFINE (scm_make_fluid, "make-fluid", 0, 0, 0,
 	    "with its own dynamic state, you can use fluids for thread local storage.")
 #define FUNC_NAME s_scm_make_fluid
 {
-  return scm_cell (scm_tc7_fluid, (scm_t_bits) next_fluid_num ());
+  return new_fluid ();
 }
 #undef FUNC_NAME