During the review of JDK-8268163 we noticed that the upgrade to full gc in VM_G1TryInitiateConcMark::doit() is never true, and so the upgrade will never happen:
The code is something like this:
} else if (g1h->should_upgrade_to_full_gc(_gc_cause)) {
_gc_succeeded = g1h->upgrade_to_full_collection();
} else {
_gc_succeeded = true;
}
with G1CollectedHeap::upgrade_to_full_gc() doing this:
if (should_do_concurrent_full_gc(_gc_cause)) {
return false;
} else if (has_regions_left_for_allocation()) {
but VM_G1TryInitiateConcMark is only ever executed with should_do_concurrent_full_gc() == true, so the result of upgrade_to_full_gc is always false.
This bug has been introduced with some refactoring of G1 VM operations inJDK-8232588.
However, there is something odd about the whole feature added inJDK-8195158 which is written as if the intention had been to *not* do this (https://bugs.openjdk.java.net/browse/JDK-8195158?focusedCommentId=14147598&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14147598) states:
" The fix (which is being worked on) is to not upgrade user initiated GCs that are supposed to be concurrent. "
and the code *adds* this functionality too.
So either this is a bug becauseJDK-8232588 disabled this functionality, or this is a bug because since the upgrade is impossible, there is superfluous code.
I think we should upgrade for user-initiated system.gc() on the expectation that at least some memory should be freed; in that case removing the first check in should_upgrade_to_full_gc() should be good (in both usages).
The code is something like this:
} else if (g1h->should_upgrade_to_full_gc(_gc_cause)) {
_gc_succeeded = g1h->upgrade_to_full_collection();
} else {
_gc_succeeded = true;
}
with G1CollectedHeap::upgrade_to_full_gc() doing this:
if (should_do_concurrent_full_gc(_gc_cause)) {
return false;
} else if (has_regions_left_for_allocation()) {
but VM_G1TryInitiateConcMark is only ever executed with should_do_concurrent_full_gc() == true, so the result of upgrade_to_full_gc is always false.
This bug has been introduced with some refactoring of G1 VM operations in
However, there is something odd about the whole feature added in
" The fix (which is being worked on) is to not upgrade user initiated GCs that are supposed to be concurrent. "
and the code *adds* this functionality too.
So either this is a bug because
I think we should upgrade for user-initiated system.gc() on the expectation that at least some memory should be freed; in that case removing the first check in should_upgrade_to_full_gc() should be good (in both usages).
- relates to
-
JDK-8232588 G1 concurrent System.gc can return early or late
-
- Resolved
-
-
JDK-8195158 Concurrent System.gc() is "upgraded" to stop-the-world System.gc()
-
- Closed
-
-
JDK-8268163 Change the order of fallback full GCs in G1
-
- Resolved
-