Re: Weak Vectors Patch

[Clinton Ebadi <clinton@unknownlamer.org>]

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

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

> For these last two things, we’d need to modify the ‘vector-ref’ and
> ‘vector-set’ instructions so that they do the right thing.  The best
> solution would be to just call scm_c_vector_{ref,set_x} when
> SCM_I_WVECTP, so that the overhead remains low for regular vectors.  Can
> you look into this?

Wingo did this.

>> I'm not sure that weak vectors should have their allocated memory marked
>> as containing no pointers, but it seems to work so I left that alone in
>> case there was some magic going on that I didn't quite grok.
>
> Yes, weak vectors are to live in pointerless memory, otherwise the
> disappearing links would not disappear.
>
> The patch looks good to me, modulo a few things:
>
>   - Could you make it 3 (or 4?) different patches, each with a test case
>     showing what is being fixed?
>
>   - If that’s fine with you, you’ll need to assign copyright to the
>     FSF.  We can arrange this off-list.

I have attached a series of three patches with reformatted GNU style
commit logs and the gnu coding style violations fixed.

I talked to wingo, and he suggested that (equal? WEAK-VECTOR VECTOR) =>
#f; I have made this so by unregistering weak vectors as arrays, and
doing additional type checking in scm_eq. I am unsure if this has
ramifications for the rest of the system however.

How do you want to handle copyright assignment?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Forbid-comparison-of-weak-and-normal-vectors.patch --]
[-- Type: text/x-diff, Size: 3148 bytes --]

From 51c78363c827ff7433bf619a4dd0c9afdbd79696 Mon Sep 17 00:00:00 2001
From: Clinton Ebadi <clinton@unknownlamer.org>
Date: Sat, 17 Apr 2010 14:16:30 -0400
Subject: [PATCH 1/3] Forbid comparison of weak and normal vectors

* libguile/vectors.c (scm_i_weak_vector_equal_p): New Function.

* libguile/eq.c (scm_equal_p): Dispatch scm_tc7_wvect to
  `scm_i_weak_vector_equal_p', and forbid comparison of normal and weak
  vectors.
---
 libguile/eq.c      |   11 +++++++++--
 libguile/vectors.c |   15 ++++++++++++++-
 libguile/vectors.h |    1 +
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/libguile/eq.c b/libguile/eq.c
index 923fa77..7debae7 100644
--- a/libguile/eq.c
+++ b/libguile/eq.c
@@ -342,9 +342,16 @@ scm_equal_p (SCM x, SCM y)
 	case scm_tc16_fraction:
           return scm_i_fraction_equalp (x, y);
         }
-    case scm_tc7_vector:
     case scm_tc7_wvect:
-      return scm_i_vector_equal_p (x, y);
+      if (SCM_LIKELY (SCM_TYP7 (y) == scm_tc7_wvect))
+	return scm_i_weak_vector_equal_p (x, y);
+      else
+	return SCM_BOOL_F;
+    case scm_tc7_vector:
+      if (SCM_LIKELY (SCM_TYP7 (y) == scm_tc7_vector))
+	return scm_i_vector_equal_p (x, y);
+      else
+	return SCM_BOOL_F;
     }
   /* Check equality between structs of equal type (see cell-type test above). */
   if (SCM_STRUCTP (x))
diff --git a/libguile/vectors.c b/libguile/vectors.c
index 321b499..e60e2c3 100644
--- a/libguile/vectors.c
+++ b/libguile/vectors.c
@@ -509,6 +509,19 @@ scm_i_vector_equal_p (SCM x, SCM y)
   return SCM_BOOL_T;
 }
 
+SCM
+scm_i_weak_vector_equal_p (SCM x, SCM y)
+{
+  /* This is slower, but necessary to catch NULLified weak references and
+     correctly hold the collector lock when accessing each element. */
+  long i;
+  for (i = SCM_I_VECTOR_LENGTH (x) - 1; i >= 0; i--)
+    if (scm_is_false (scm_equal_p (scm_c_vector_ref (x, i),
+				   scm_c_vector_ref (y, i))))
+      return SCM_BOOL_F;
+  return SCM_BOOL_T;
+}
+
 
 SCM_DEFINE (scm_vector_move_left_x, "vector-move-left!", 5, 0, 0, 
             (SCM vec1, SCM start1, SCM end1, SCM vec2, SCM start2),
@@ -622,7 +635,7 @@ vector_get_handle (SCM v, scm_t_array_handle *h)
 
 /* the & ~2 allows catching scm_tc7_wvect as well. needs changing if you change
    tags.h. */
-SCM_ARRAY_IMPLEMENTATION (scm_tc7_vector, 0x7f & ~2,
+SCM_ARRAY_IMPLEMENTATION (scm_tc7_vector, 0x7f,
                           vector_handle_ref, vector_handle_set,
                           vector_get_handle)
 SCM_VECTOR_IMPLEMENTATION (SCM_ARRAY_ELEMENT_TYPE_SCM, scm_make_vector)
diff --git a/libguile/vectors.h b/libguile/vectors.h
index 3746e90..f426d0c 100644
--- a/libguile/vectors.h
+++ b/libguile/vectors.h
@@ -74,6 +74,7 @@ SCM_API SCM *scm_vector_writable_elements (SCM vec,
 #define SCM_I_VECTOR_LENGTH(x) (((size_t) SCM_CELL_WORD_0 (x)) >> 8)
 
 SCM_INTERNAL SCM  scm_i_vector_equal_p (SCM x, SCM y);
+SCM_INTERNAL SCM  scm_i_weak_vector_equal_p (SCM x, SCM y);
 
 /* Weak vectors share implementation details with ordinary vectors,
    but no one else should.  */
-- 
1.6.5.7


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: 0002-Register-disappearing-links-when-initializing-weak-v.patch --]
[-- Type: text/x-diff, Size: 2350 bytes --]

From cecef73d94a59f5932e28e938145700019203801 Mon Sep 17 00:00:00 2001
From: Clinton Ebadi <clinton@unknownlamer.org>
Date: Sat, 17 Apr 2010 14:21:43 -0400
Subject: [PATCH 2/3] Register disappearing links when initializing weak vectors

* libguile/vectors.c (scm_c_vector_set_x): Unregister old weak
  link when assigning to a weak vector.
  (scm_i_make_weak_vector): Register elements as disappearing links
  to fill value.
  (scm_i_make_weak_vector_from_list): Register elements as
  disappearing links.
---
 libguile/vectors.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/libguile/vectors.c b/libguile/vectors.c
index e60e2c3..cca57cd 100644
--- a/libguile/vectors.c
+++ b/libguile/vectors.c
@@ -274,10 +274,11 @@ scm_c_vector_set_x (SCM v, size_t k, SCM obj)
       if (k >= SCM_I_VECTOR_LENGTH (v))
 	scm_out_of_range (NULL, scm_from_size_t (k)); 
       (SCM_I_VECTOR_WELTS(v))[k] = obj;
-      if (SCM_I_WVECTP (v))
+      if (SCM_I_WVECTP (v) && !SCM_IS_WHVEC (v))
 	{
 	  /* Make it a weak pointer.  */
 	  GC_PTR link = (GC_PTR) & ((SCM_I_VECTOR_WELTS (v))[k]);
+	  GC_unregister_disappearing_link (link);
 	  SCM_I_REGISTER_DISAPPEARING_LINK (link, obj);
 	}
     }
@@ -292,10 +293,11 @@ scm_c_vector_set_x (SCM v, size_t k, SCM obj)
 	  k = SCM_I_ARRAY_BASE (v) + k*dim->inc;
 	  (SCM_I_VECTOR_WELTS (vv))[k] = obj;
 
-	  if (SCM_I_WVECTP (vv))
+	  if (SCM_I_WVECTP (vv) && !SCM_IS_WHVEC (vv))
 	    {
 	      /* Make it a weak pointer.  */
 	      GC_PTR link = (GC_PTR) & ((SCM_I_VECTOR_WELTS (vv))[k]);
+	      GC_unregister_disappearing_link (link);
 	      SCM_I_REGISTER_DISAPPEARING_LINK (link, obj);
 	    }
 	}
@@ -420,7 +422,10 @@ scm_i_make_weak_vector (scm_t_bits type, SCM size, SCM fill)
   base = SCM_I_WVECT_GC_WVELTS (wv);
 
   for (j = 0; j != c_size; ++j)
-    base[j] = fill;
+    {
+      base[j] = fill;
+      SCM_I_REGISTER_DISAPPEARING_LINK ((GC_PTR) base + j, (GC_PTR) SCM_UNPACK(fill));
+    }
 
   return wv;
 }
@@ -442,6 +447,7 @@ scm_i_make_weak_vector_from_list (scm_t_bits type, SCM lst)
        scm_is_pair (lst);
        lst = SCM_CDR (lst), elt++)
     {
+      SCM_I_REGISTER_DISAPPEARING_LINK((GC_PTR) elt, (GC_PTR) SCM_UNPACK(SCM_CAR(lst)));
       *elt = SCM_CAR (lst);
     }
 
-- 
1.6.5.7


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.4: 0003-Reference-weak-vectors-with-the-allocator-lock-held.patch --]
[-- Type: text/x-diff, Size: 2906 bytes --]

From af6252f00326542cfa052ec9c2a99477cd1b1d7e Mon Sep 17 00:00:00 2001
From: Clinton Ebadi <clinton@unknownlamer.org>
Date: Sat, 17 Apr 2010 14:23:16 -0400
Subject: [PATCH 3/3] Reference weak vectors with the allocator lock held

* libguile/vectors.c (do_weak_vector_ref): New function.
  (scm_c_vector_ref): Call `do_weak_vector_ref' with the allocator
  lock held when referencing weak vectors.
---
 libguile/vectors.c |   54 ++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/libguile/vectors.c b/libguile/vectors.c
index cca57cd..5788ec1 100644
--- a/libguile/vectors.c
+++ b/libguile/vectors.c
@@ -202,22 +202,46 @@ scm_vector_ref (SCM v, SCM k)
 }
 #undef FUNC_NAME
 
+struct t_weak_vector_ref_args
+{
+  SCM vector;
+  size_t k;
+};
+
+/* Accessing weak pointers *must* be done with the allocator lock held. */
+static void * 
+do_weak_vector_ref (void* data)
+{
+  struct t_weak_vector_ref_args *args = (struct t_weak_vector_ref_args *) data;
+  register SCM elt = (SCM_I_VECTOR_ELTS(args->vector))[args->k];
+
+  if (elt == SCM_PACK (NULL))
+    /* ELT was a weak pointer and got nullified by the GC.  */
+    return SCM_BOOL_F;  
+
+  return elt;
+}
+
 SCM
 scm_c_vector_ref (SCM v, size_t k)
 {
   if (SCM_I_IS_VECTOR (v))
     {
-      register SCM elt;
-
       if (k >= SCM_I_VECTOR_LENGTH (v))
 	scm_out_of_range (NULL, scm_from_size_t (k));
-      elt = (SCM_I_VECTOR_ELTS(v))[k];
 
-      if ((elt == SCM_PACK (NULL)) && SCM_I_WVECTP (v))
-	/* ELT was a weak pointer and got nullified by the GC.  */
-	return SCM_BOOL_F;
+      if (SCM_I_WVECTP (v) && !SCM_IS_WHVEC (v))
+	{
+	  struct t_weak_vector_ref_args args;
+	  args.vector = v;
+	  args.k = k;
 
-      return elt;
+	  return GC_call_with_alloc_lock (do_weak_vector_ref, &args);
+	}
+      else
+	{
+	  return (SCM_I_VECTOR_ELTS(v))[k];
+	}
     }
   else if (SCM_I_ARRAYP (v) && SCM_I_ARRAY_NDIM (v) == 1)
     {
@@ -225,18 +249,20 @@ scm_c_vector_ref (SCM v, size_t k)
       SCM vv = SCM_I_ARRAY_V (v);
       if (SCM_I_IS_VECTOR (vv))
 	{
-	  register SCM elt;
-
 	  if (k >= dim->ubnd - dim->lbnd + 1)
 	    scm_out_of_range (NULL, scm_from_size_t (k));
 	  k = SCM_I_ARRAY_BASE (v) + k*dim->inc;
-	  elt = (SCM_I_VECTOR_ELTS (vv))[k];
 
-	  if ((elt == SCM_PACK (NULL)) && (SCM_I_WVECTP (vv)))
-	    /* ELT was a weak pointer and got nullified by the GC.  */
-	    return SCM_BOOL_F;
+	  if (SCM_I_WVECTP (vv) && !SCM_IS_WHVEC (vv))
+	    {
+	      struct t_weak_vector_ref_args args;
+	      args.vector = vv;
+	      args.k = k;
+
+	      return GC_call_with_alloc_lock (do_weak_vector_ref, &args);
+	    }
 
-	  return elt;
+	  return (SCM_I_VECTOR_ELTS (vv))[k];
 	}
       scm_wrong_type_arg_msg (NULL, 0, v, "non-uniform vector");
     }
-- 
1.6.5.7


[-- Attachment #1.5: Type: text/plain, Size: 267 bytes --]





-- 
Corinne: this is why we should have designated bath buddies
Corinne: to get places you cant reach because youre slippery and in
         case you get a lil tooo slippery and crack your head open
         someone can call the coast guard and save you

[-- Attachment #2: Type: application/pgp-signature, Size: 229 bytes --]