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

[Tony Printezis]

Bengt,

Looks good.

Tony

Bengt Rutisson wrote:
>
> Ramki and Tony, thanks for your comments!
>
> Yasumasa has made the changes that you requested and I have put 
> together an updated webrev:
>
> http://cr.openjdk.java.net/~brutisso/7015169/webrev.01/
>
> We discussed whether or not the asserts in 
> VM_GC_Operation::doit_prologue() are really necessary now that the 
> cause is always set by the contructor. We decided to leave the asserts 
> in for the webrev. If anyone thinks they are unnecessary we'll remove 
> them.
>
> We also found that GCCause::is_for_full_collection() in gcCause.cpp 
> was dead code and removed this method as part of this change.
>
> Thanks,
> Bengt
>
>
> On 2011-01-28 21:47, Y. Srinivas Ramakrishna wrote:
>> 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
>>>>
>>
>