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

[Y. Srinivas Ramakrishna]

On 1/28/2011 12:32 PM, Tony Printezis wrote:
> 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?

Ah, but of course :-)  (or as the American expression goes: "Doh!").

-- ramki

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