At 3:21 AM -0500 3/25/02, Michel J Lambert wrote:
>Below is a cleanup patch for a comment or two, some flags missing, that I
>did while reading through it all in an attempt to figure it all out.

Thanks for the code review--you've found a number of bugs and missing bits.

We ought to see about arranging for more like this at the various 
conferences and users group meetings. It's really useful.

>I was impressed with how straightforward all the code was, but there were
>a few things that confused me. If you answer these, I can provide
>documentation patches or cleanups to help clarify things that I found
>confusing. (And I'm assuming others would find confusing.)
>
>The biggest thing was the interaction and difference between strings and
>buffers in the GC system. First, there are three pools in the headers and
>the code. Then I saw that buffers and PMCs get better treatment than
>strings with their mark_*_unused, trace_active_*,s and free_unused_*. To
>top it off, the buffer versions of those functions deal with the STRING
>arenas and registers, which seems a bit counterintuitive.

Right, three pools. Soon four or five, I think.

Pools are currently of two classes: PMCs, and Buffer-ish things. 
(Including STRINGs)

>Finally, trace_active_PMCs seems to be the only function to handle
>buffers. Looking at the amount of similarity between that and
>trace_active_buffers, I think it might help speed to combine those two
>functions together, to help avoid the duplicate loops, and to make it a
>bit easier to follow.

Things are the way they are because I finally had the realization 
that there may be active Buffer things in registers that aren't 
referred to by PMCs. I know I said we weren't going to do that, but, 
well, we are. I'm still holding out on the requirement that all 
Buffers stored in variables must be referred to by PMCs.

>I think I understand it all now, but I'd like to check to make sure I'm
>right. All strings are, and can be treated as, buffers, since they are
>referenced by pointer only.

Right.

>The cleanup routines know that it's a string,
>and free each to the appropriate pool.

Sort of. The "mark me as unused" routine don't know, which is why 
they just set a flag. The routines that trace arenas of headers know 
what type of header is in the arena, though, and puts things back as 
needed. (I'm thinking of unifying the arena tracing so that each 
arena holds a pointer to its free pool and a header size field. For 
the next version, perhaps)

>Buffers have no high-level access,
>and exist only as part of PMCs, so they can only be freed from there.

That's changed. Buffers can also be referred to by just an S 
register, S register stack entry, or generic stack entry.

>If my understanding is correct, then what is the purpose of
>find_dead_strings and dead_string_run? They both are not implemented and
>not called from anywhere. Are they intended to be implemented at some
>point, or should they disappear?

dead_string_run is useless, and needs tossing. (So I did :) I think 
find_dead_strings is too, as the PMC and S register tracing find 
those.

Looks like I never finished the code to put unused Buffers back in 
their free pool. Nobody's noticed, because the String stuff is done 
and that's all we have so far.

>ppd09 talks about custom DOD and GC functions, and support for buffers of
>buffers, but I'm unable to find code that supports either. Just checking
>those are still tbd.

Neither are implemented yet. I thought they were, but I think I blew 
away and refreshed my working copy with them before checking the 
things in. That needs re-implementing.

>Where does the 'Not yet implemented' come into play for free_unused_PMCs?
>It looks implemented and working to me. (Although I did not debug it to
>see if it is being called and working.)

D'oh! Bit rot. Comment was out of date--now fixed.

>@@ -110,7 +110,7 @@
>    /* We return system memory if we've got no interpreter yet */
>    if (NULL == interpreter) {
>      return_me = mem_sys_allocate(sizeof(PMC));
>-    return_me->flags = PMC_live_FLAG;
>+    return_me->flags = PMC_live_FLAG | BUFFER_sysmem_FLAG;

This isn't right, but the rest are.

-- 
                                         Dan

--------------------------------------"it's like this"-------------------
Dan Sugalski                          even samurai
[EMAIL PROTECTED]                         have teddy bears and even
                                       teddy bears get drunk

Reply via email to