Review Request (s) - 7015169 GC Cause not always set

[Y. Srinivas Ramakrishna]

Hi Bengt --

This looks generally good to me, but i was wondering if we could somehow make this
less "error-prone" in the following sense. There appears to be nothing that
forces a gc op to actually set the _gc_cause value. We rely on each gc op
c'tor to initialize the field, and for each doit() to use a GCCauseSetter
to export the value into the heap when it runs. It would be nice if
we could verify that each op had its _gc_cause field initialized in the
c'tor, or at least before its doit() ran, and for one place to export
the field rather than each subclass remembering to do it. For the former,
the only thing i can think of off the top of my head is a hack: have the
base class with that field initialize _gc_cause to "_no_cause" in its c'tor,
and to check in its d'tor that the value has been changed to something
different. We can't have the base class's c'tor check this
because the subclass c'tors would not have run by then.
For the latter, the only thing i could think of
feels somewhat awkward too: have the VMThread check if an STW op is a
GC op (we could introduce an attribute to check for this for any vm op),
and do the GCCauseSetter thing in the VM thread before calling the doit()
for each such op; may be the VMThread could at that time check that the
cause field was not a "not set" value. Unfortunately, this kind of leaks
these attributes and checks out of the vm op classes and so feels awkward.
Perhaps someone might have better ideas on how that might be achieved.
(Of course such a "clean-up" could happen separately rather than needing
to be done in this changeset.)

Other than those very high level comments, your changes look good to me
otherwise wrt their specifics. I did not of course check that we got
all the missing initializations which is one reason why i raised this
issue as well as to ease future changes and new gc vm ops.

-- ramki

On 1/27/2011 4:00 AM, Bengt Rutisson wrote:
>
> Hi Everybody,
>
> Following up on a mail discussion called "jstat LGCC column shows" on this mailing list. Yasumasa
> Suenaga has agreed to contribute the patch that he made to the OpenJDK project. I am helping him to
> get it pushed into the OpenJDK repositories.
>
> We created bug 7015169 for this issue. It is not available on http://bugs.sun.com yet, but I hope it
> gets published there soon.
>
> To push we need a couple of reviews. Here is the webrev:
> http://cr.openjdk.java.net/~brutisso/7015169/webrev.00/
>
> The problem was that _gc_cause was not set by all VM_GC_Operation that can request a GC. This was
> visible through jstat and with Yasumasa's fix the LGCC columns shows the correct GC cause.
>
> Thanks,
> Bengt