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

[Y. S. Ramakrishna]

Looks good to me. (PS: At some point in the future, we'll probably
want to think about how best to consolidate the export of gc cause
into the heap, rather than have each vm gc op doit() as is the case now;
the second point i made in a previous email below. For instance,
we could s]restructure vm gc ops so the real work is done in
a virtual doit_work() method, while the doit() of the vm gc op
class does the export and then calls the virtual doit_work(), but that's
a possible cleanup for the future and a different debate.)

-- ramki

On 02/02/11 23:18, 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
>>>>
>>
>