CRR (S): 7129892: G1: explicit marking cycle initiation might fail to initiate a marking cycle

[Tony Printezis]

Bengt,

Inline.

On 02/08/2012 06:50 AM, Bengt Rutisson wrote:
>
>> I wrote a test that exposes the issue (when 
>> +ExplicitGCInvokesConcurrent is used) and I attached it to the CR (it 
>> initiates marking cycles constantly, I just compare the number of 
>> System.gc()'s to the number of actual marking cycles in the log). I 
>> did the first implementation based on the first version of your "hum 
>> alloc can start a conc cycle" fix and I had to update it based on the 
>> changes you pushed afterwards. I just reran the test to make sure 
>> all's still well, and it is:
>>
>> vanilla: tried to initiate 2610 cycles, 2595 cycles actually initiated
>>
>> with fix: tried to initiate 2613 cycles, 2613 cycles actually initiated
>
> Excellent! Thanks. Sorry for not checking the CR properly before asking.

Np (I should have mentioned the test in the code review request). I 
couldn't come up with a straightforward way to also test the fix when 
cycles are initiated by humongous allocations. But, given that that code 
path also calls collect() I think exercising the code with 
+ExplicitGCInvokesConcurrent should be enough.

>> FWIW, I like the version with the method even if it's just for the 
>> extra comments which might be helpful to whoever reads the code in 
>> the future.
>
> Well, we don't really need the switch to have comments, do we? Also, I 
> would prefer to have the comments in should_do_concurrent_full_gc(). 
> That's where I would look for the information that the comments provide.
>
> Infact, that method already has a comment in the hpp file:
>
>   // It decides whether an explicit GC should start a concurrent cycle
>   // instead of doing a STW GC. Currently, a concurrent cycle is
>   // explicitly started if:
>   // (a) cause == _gc_locker and +GCLockerInvokesConcurrent, or
>   // (b) cause == _java_lang_system_gc and +ExplicitGCInvokesConcurrent.
>   // (c) cause == _g1_humongous_allocation
>   bool should_do_concurrent_full_gc(GCCause::Cause cause);
>
> Mabye just make this comment more detailed?

Do you like this version better?

http://cr.openjdk.java.net/~tonyp/7129892/webrev.3/

I also noticed that we were calling 
SharedHeap::heap()->total_collections() from a few places in 
G1CollectedHeap and we can call total_collections() directly given that 
G1CH extends SharedHeap. I simplified those.

Tony

>>> Finally a coding standard question regarding switch statements. The 
>>> hotspot code in general is not consistent in this case, and neither 
>>> is the GC code or even the G1 code. But should the "case" statements 
>>> be indented one level under the "switch" statement? To me that makes 
>>> sense since the switch statement starts a new code block. I assume 
>>> you have a different opinion since you wrote it differently, but 
>>> what is the rational behind it?
>>>
>>> To exemplify. This:
>>>
>>>   switch (cause) {
>>>   case GCCause::_gc_locker:               return 
>>> GCLockerInvokesConcurrent;
>>>   case GCCause::_java_lang_system_gc:     return 
>>> ExplicitGCInvokesConcurrent;
>>>   case GCCause::_g1_humongous_allocation: return true;
>>>   default:                                return false;
>>>   }
>>>
>>> would be easier for me to read if it was:
>>>
>>>   switch (cause) {
>>>     case GCCause::_gc_locker:               return 
>>> GCLockerInvokesConcurrent;
>>>     case GCCause::_java_lang_system_gc:     return 
>>> ExplicitGCInvokesConcurrent;
>>>     case GCCause::_g1_humongous_allocation: return true;
>>>     default:                                return false;
>>>   }
>>
>> I'm OK either way and I do think the latter is a bit nicer. But by 
>> default emacs does not indent so I went with it. I'll add the 
>> indentation.
>
> Great! :-)
>
> Bengt
>
>>> Anyway, just ,minor stuff. So, in general: ship it!
>>
>> Thanks!
>>
>> Tony
>>
>>> Bengt
>>>
>>> On 2012-01-26 20:52, Tony Printezis wrote:
>>>> Hi all,
>>>>
>>>> Can I please have a couple of code reviews for the following change?
>>>>
>>>> http://cr.openjdk.java.net/~tonyp/7129892/webrev.1/
>>>>
>>>> The issue is that a GC attempt that's supposed to explicitly start 
>>>> a concurrent marking cycle might be unsuccessful (as another GC 
>>>> might get scheduled first) which will prevent the cycle from 
>>>> starting. The idea is to retry such unsuccessful attempts.
>>>>
>>>> I also changed should_do_concurrent_full_gc() from an if-statement 
>>>> to a switch statement. I discussed it with Bengt (the last person 
>>>> to modify that method) and we both think that the switch statement 
>>>> is more readable.
>>>>
>>>> BTW, I did a couple of iterations of this fix to address the 
>>>> slightly different approach Bengt took in the cycle initiation 
>>>> after hum allocation code  (i.e., start the cycle before the 
>>>> allocation, not after). In the end the current version of 
>>>> retry_unsuccessful_concurrent_full_gc(), a new method I added, 
>>>> always returns true for all causes. I'm inclined to leave the 
>>>> switch in, even just for the comments per cause. I could be 
>>>> persuaded to replace it with return true; statement though.
>>>>
>>>> Tony
>>>>
>>>
>