[Mono-devel-list] [PATCH] GC Profiling Interfaces

[Ben Maurer]

On Sun, 2005-01-23 at 12:16 +0100, Paolo Molaro wrote:
> There are several problems with the patch.
> First, it's not 64 bit clean. You need to use at least size_t
> for heap sizes, but maybe it's better to just use a 64 bit int
> unconditionally.

Yes, I should. Fixing now.

> The interface is wrong, IMHO. First, mono_profiler_mark_set() is misnamed:
> it should be mono_object_is_alive () and it should not be included in
> the profiler interface.

I wanted to keep it in the profiler because it is only useful during the
profiler callback.

>  But even with these changes, I'm not sure we
> can support that interface and the way you use it. Think for example a GC
> that sets the mark bits in the least significant bits of the object header:
> the function can be implemented, but you can't use the object directly.
> I don't want to export assumptions on a GC implementation in a public
> interface.

This interface is not designed to be useful for a different gc impl. It
doesn't handle moving objects *at all*.

A profiler for Mono 1.2 is unlikely to be useful for 2.0 (Or whenever we
get a moving gc). Even if the api were designed so that the profiler
were correct, the things one wants to look at in each impl are very
different.

I'd also like to get boehm specific stuff (eg, number of blocks
blacklisted). Maybe these functions need to be put somewhere else...

> As for knowing that an object died vs knowing which objects are alive, it's
> a simple difference which you could do when post processing, too, if needed.
> Either way, we may consider exporting such an ugly interface only if
> the overhead with the heap walk is significant.

Maybe, considering dependency on the gc impl, we should consider this a
private contract with the gc profiler? 

> > +/* Slow/general mark bit manipulation: */
> > +GC_API int GC_is_marked GC_PROTO((char * p));
> > +GC_API void GC_clear_mark_bit GC_PROTO((char * p));
> > +GC_API void GC_set_mark_bit GC_PROTO((char * p));
> 
> Why do you export GC_clear_mark_bit and GC_set_mark_bit even if they are not used.
> The fact that they are in the gc_priv.h header should be a strong enough
> hint that care should be applied when exporting them.

Ooops, that is some over zealous c+p.

> > Index: mono/metadata/profiler.c
> > ===================================================================
> > --- mono/metadata/profiler.c	(revision 39325)
> > +++ mono/metadata/profiler.c	(working copy)
> > @@ -12,6 +12,7 @@
> >  #ifdef HAVE_BACKTRACE_SYMBOLS
> >  #include <execinfo.h>
> >  #endif
> > +#include <mono/os/gc_wrapper.h>
> 
> It's not acceptable to depend on the guts of the GC in this file.
> Don't use any Boehm GC specific call here, we need to provide an API,
> not hack the code until it seems to work.

Ok, I'll abstract the code out to the gc code.

> Note that you can use mono_gc_get_total_memory() when you want and your code
> won't depend on the GC implementation details.

Ok.

> This interface is wrong: it should provide GC events such as GC start
> and GC end and it should include the generation number.
> 
> enum {
> 	/* maybe also add suspend and resume events */
> 	MONO_GC_EVENT_START,
> 	MONO_GC_EVENT_RECLAIM,
> 	MONO_GC_EVENT_END
> };
> 
> typedef void (*MonoProfileGCFunc)         (MonoProfiler *prof, int event_type, int gen_num);
> 
> Of course for the current GC the generation number will be always 0, but
> the interface will work with the new GC. Note also that the generation number
> is not needed, since you can easily keep your counter when the MONO_GC_EVENT_START
> event is received.

So MONO_GC_EVENT_RECLAIM would be like the one that allows me to see the
mark bits?

-- Ben