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 08:06:11 UTC 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