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

[Bengt Rutisson]

Tony,

On 2012-02-07 18:38, Tony Printezis wrote:
> Bengt,
>
> Thanks for looking at it, inline.
>
> On 02/07/2012 05:03 AM, Bengt Rutisson wrote:
>>
>> Hi Tony,
>>
>> Overall this looks good.
>>
>> Did you do any testing to see that it behaves as you expect now?
>
> 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.

>> Some style related comments:
>>
>> You mentioned that you can be persuaded to remove the switch that 
>> always returns true from retry_unsuccessful_concurrent_full_gc(). I'm 
>> in favor of removing that. Actually I think that if we do that we 
>> could inline the the remaining part of 
>> retry_unsuccessful_concurrent_full_gc() inside the collect() method. 
>> That would make it more readable to me. 
>
> Actually, we don't need to inline anything. The change becomes this 
> (ignore the 0 lines changed in the .hpp file, this is a side-effect of 
> having two MQ patches stacked):
>
> http://cr.openjdk.java.net/~tonyp/7129892/webrev.2/
>
> (note that only when the GC cause is appropriate and any related 
> arguments are set appropriately we try to initiate a conc cycle, so we 
> don't need to recheck)

As you noted in your next email we still need one check.

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

>> The name "retry_unsuccessful_concurrent_full_gc" is complicated 
>> enough that I would have to go and look what it does anyway. Since it 
>> will only be one line if it is inlined I would prefer that.
>
> You can propose an alternative. :-) I couldn't come up with one...

I still think we should just inline it and not have the method.

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