Re: GUILE_MAX_HEAP_SIZE

[Ludovic Courtès]

Hi Han-Wen,

Han-Wen Nienhuys <address@hidden> writes:

> I did a cleanup round over the GC code - see the dev/hanwen branch on
> sv.gnu.org ; It seems to work ok, there is a just one annoyance: there
> is a commented out assert that works most of the time, but once in a
> while is off by a small amount in either direction.  Comments/insights
> appreciated.
>
> Unfortunately, I am not the patient type, and also not aflush with
> free time, so the changes come as one big bunch, with layout,
> refactoring and some bugfixes all lumped together.
>
> (I intend to squash into a single commit before pushing to master).

First of all, thanks for your work (I know it's not so much fun to hack
the GC), but I feel unhappy with your commit to both `master' and
`branch_release-1-8'.

As you probably know, it's not customary to commit anything non-trivial
there, especially in the stable branch, without waiting for review, and
while being conscious that the code is slightly broken (!).  We may need
to revert them before going any further.

I'll comment your commits in chronological order.  As their logs are
quite terse, I may be missing the point of some of them.


  * 2072309c1c39cf193687cd895348d2f817537a3e
    Whitespace and formatting fixes.

    I wouldn't do that in tricky code as it makes it harder to audit
    (I'm feeling like a broken record...).

  * 01621bf62ec16cb62260f0b7c9e926793718fd6d
    Include min-yields in gc-stats output.

    Good idea.

  * 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?

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

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

  * e89b7b36259edb20f60efc0e3e11fa83e5b35b89
    Remove unused macro UNMARKED_CELL_P()

    OK.

  * 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?

  * b474ac33ee0e47ab14306c218cb060667f9af2db
    Remove comments about removed variables.

    OK.

Stopping here for now.

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.

Thanks,
Ludovic.