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

[Tony Printezis]

Ramki,

Instead of the c'or of the base class setting the _cause field to a 
known value and checking later that the value has been changed, why 
don't we introduce a cause field on the c'or of the base class, which is 
then assigned to _cause, to force all subclasses to pass a value to it?

Tony

Y. Srinivas Ramakrishna wrote:
> 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
>