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

[Tony Printezis]

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

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

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.

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

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

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