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

[Bengt Rutisson]

Hi Tony,

Overall this looks good.

Did you do any testing to see that it behaves as you expect now?

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

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

Anyway, just ,minor stuff. So, in general: ship it!
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
>