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

Tony Printezis tony.printezis at oracle.com
Thu Feb 9 11:16:52 PST 2012


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