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

Bengt Rutisson bengt.rutisson at oracle.com
Fri Feb 10 00:06:11 PST 2012


Tony,

New webrev looks great! Thanks for updating it again.

Bengt


On 2012-02-09 20:16, Tony Printezis wrote:
> Bengt,
>
> Inline.
>
> On 02/09/2012 04:05 AM, Bengt Rutisson wrote:
>>
>> Tony,
>>
>> The new webrev looks good! Ship it!
>
> Thanks. :-)
>
>> Good idea to clean up the calls to 
>> SharedHeap::heap()->total_collections().
>>
>> There are a couple of more places in g1CollectedHeap.cpp where we 
>> unnecessarily use the  SharedHeap:: prefix. Don't know if you want to 
>> change them as well, of if you think they are good for readability. 
>> Below is a small patch to remove those usages as well. I'll leave it 
>> up to you to decide on using it or not. I'm fine with the latest 
>> webrev as it is.
>
> I adopted your changes to drop the SharedHeap:: prefix for 
> ScanningOption and friends (and there was an extra instance of that in 
> the .hpp file). I think I'll leave
>
> SharedHeap::process_weak_roots(root_closure, &roots_in_blobs, 
> non_root_closure);
>
> as is if that's OK. G1 also has a process_weak_roots() method and, 
> even though it has a different parameter list, I think the ShredHeap:: 
> here makes the code a bit more readable (IMHO). It will also guard 
> against future "surprises" if we change the parameter list of either 
> process_weak_roots().
>
> Latest full webrev here:
>
> http://cr.openjdk.java.net/~tonyp/7129892/webrev.4/webrev.all/
>
> And these are just the above changes:
>
> http://cr.openjdk.java.net/~tonyp/7129892/webrev.4/webrev.1.G1Latest/
>
> Tony
>
>> Thanks,
>> Bengt
>>
>> diff --git a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp 
>> b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
>> --- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
>> +++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
>> @@ -3171,12 +3171,12 @@
>>
>>      // We apply the relevant closures to all the oops in the
>>      // system dictionary, the string table and the code cache.
>> -    const int so = SharedHeap::SO_AllClasses | 
>> SharedHeap::SO_Strings | SharedHeap::SO_CodeCache;
>> +    const int so = SO_AllClasses | SO_Strings | SO_CodeCache;
>>
>>      process_strong_roots(true,      // activate StrongRootsScope
>>                           true,      // we set "collecting perm gen" 
>> to true,
>>                                      // so we don't reset the dirty 
>> cards in the perm gen.
>> -                         SharedHeap::ScanningOption(so),  // roots 
>> scanning options
>> +                         ScanningOption(so),  // roots scanning options
>> &rootsCl,
>> &blobsCl,
>> &rootsCl);
>> @@ -4756,7 +4756,7 @@
>>  void
>>  G1CollectedHeap::
>>  g1_process_strong_roots(bool collecting_perm_gen,
>> -                        SharedHeap::ScanningOption so,
>> +                        ScanningOption so,
>>                          OopClosure* scan_non_heap_roots,
>>                          OopsInHeapRegionClosure* scan_rs,
>>                          OopsInGenClosure* scan_perm,
>> @@ -4828,7 +4828,7 @@
>>  G1CollectedHeap::g1_process_weak_roots(OopClosure* root_closure,
>>                                         OopClosure* non_root_closure) {
>>    CodeBlobToOopClosure roots_in_blobs(root_closure, /*do_marking=*/ 
>> false);
>> -  SharedHeap::process_weak_roots(root_closure, &roots_in_blobs, 
>> non_root_closure);
>> +  process_weak_roots(root_closure, &roots_in_blobs, non_root_closure);
>>  }
>>
>>  // Weak Reference Processing support
>>
>> On 2012-02-08 15:47, Tony Printezis wrote:
>>> 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
>>>>>>>
>>>>>>
>>>>
>>



More information about the hotspot-gc-dev mailing list