CRR (S): 7129892: G1: explicit marking cycle initiation might fail to initiate a marking cycle
Bengt Rutisson
bengt.rutisson at oracle.com
Thu Feb 9 09:05:17 UTC 2012
Tony,
The new webrev looks good! Ship it!
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.
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