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 19:16:52 UTC 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