Re: Weak Vectors Patch

[Ludovic Courtès]

Hi!

Clinton Ebadi <address@hidden> writes:

> Attached is a patch that should improve weak vectors and prevent
> `vector-ref' from segfaulting on them after a GC.
>
> The root of the problem was that weak vectors were being allocated as
> containing no pointers *and* disappearing links were not being
> registered for initial elements. If you, however, created an empty weak
> vector and then `vector-set!' the elements things were fine.

Indeed, good catch!

Also, disappearing links were not being unregistered when setting a new
value with ‘vector-set!’, which your patch fixes.

Lastly, disappearing links where consulted without having the alloc
lock, which your patch fixes as well.

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?

> 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.

Minor remarks in-line:

> From a20475f1f9643093ea96118a71826623ecf15220 Mon Sep 17 00:00:00 2001
> From: Clinton Ebadi <address@hidden>
> Date: Thu, 1 Apr 2010 13:11:10 -0400
> Subject: [PATCH] Fix weak vectors
>  * libguile/vectors.c (scm_i_make_weak_vector,
>    scm_i_make_weak_vector_from_list): Register elements as disappearing links.
>  * libguile/vectors.c (do_weak_vector_ref): New function to access an
>    element of a weak vector while the allocator lock is held.

“New function” is enough.

> +/* Accessing weak pointers *must* be done with the allocator lock held */
> +static void* 
> +do_weak_vector_ref (void* data)

Please add a space before ‘*’ and punctuate sentences.

> +      SCM_I_REGISTER_DISAPPEARING_LINK((GC_PTR) base + j, (GC_PTR) 
> SCM_UNPACK(fill));

Please leave a space before ‘(‘.

Thanks!

Ludo’.