Re: GUILE_MAX_HEAP_SIZE

[Han-Wen Nienhuys]

Ludovic Courtès escreveu:
>   * 01621bf62ec16cb62260f0b7c9e926793718fd6d
>     Include min-yields in gc-stats output.
> 
>     Good idea.

No, bad idea, and removed in subsequent commit.

> 
>   * 51ef99f7fa9fb766fbb48619fc5863ab9914591d
>     Fix memory corruption issue with hell[] array: realloc/calloc need to
>     factor in sizeof(scm_t_bits)
> 
>     -  hell = scm_malloc (hell_size);
>     +  hell = scm_calloc (hell_size * sizeof(scm_t_bits));
> 
>     Good catch, but it should read:
> 
>        hell = scm_calloc (hell_size * sizeof (*hell));
> 
>     `sizeof (*hell)' is actually `sizeof (scm_t_bits *)', which is equal
>     to `sizeof (scm_t_bits)', but using `sizeof (*hell)' is clearer and
>     less error-prone.
> 
>     Besides, is that code only used when the one changes the class of an
>     instance?  How did you trigger it?

valgrind. Fixed.


>   * b61b5d0ebea924ee4b3930b2cba72e282d4751c7
>     Do not include private-gc.h in srfi-60.
> 
>     Hmm, well, we really need an `SCM_MIN ()' somewhere.  I'd rather
>     duplicate its definition than expand it as you did here, since that
>     makes the code rather unreadable.

I called private-gc.h private for a reason.   Please do not include it 
unless you are called libguile/gc*c; feel free to transplant SCM_MIN to
someplace else.


>   * 569aa529d5379f3c942fa6eb01e8a1ad48ba9f77
>     Use word_2 to store mark bits for freeing structs and vtables in the
>     correct order.
> 
>     Can you explain this?  Suppose we have struct S whose vtable is V;
>     V cannot be swept in the same GC cycle as S since it's still
>     referenced by S.  Thus, I don't understand the need for
>     special-casing here.

Freeing S requires a function stored in V. Hence the subordering.  It used to 
abuse the GC bits for that, causing warning bells to go off in the new code.
(An alternative is reset the bits after they have been used.)

>   * d09752ffd17688b33a1e828cf4c11f66b86c3c3c
>     Introduce scm_i_marking to detect when GC mark bits are touched
>     outside of marking stage.
> 
>     `ensure_marking ()' must be `static'.  The definition of
>     `scm_i_marking' clearly doesn't belong in a header.  Besides, all
>     this is unused, so what's the point?

I'm not sure where to put the code, perhaps in a ifdef DEBUG or something:
the point was to extend SCM_GC_SET_MARK with ensure_marking() to catch illegal 
use of the mark bits.

> It's highly unpleasant to have to review things after the fact.  Looks
> like both branches are on hold until we've sorted that out, and that
> sucks.  Branching exists precisely so that people can work on separate
> topics without interfering with each other.

By rolling back changes, you have just robbed me of the opportunity to see
what it was I did wrong.  Can you remind me again of the sha1 of the stable 
branch?  AFAICR I only pushed trivial fixes there.

Also, if a core contributor apparently need some sort of review process to push 
code they feel comfortable with, can you please post a link to the process?

It's not that I see many reviews of your commits on the list passing by. 

-- 
 Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen