RFR: 8249676: [REDO] G1 incorrectly limiting young gen size when using the reserve can result in repeated full gcs

[Kim Barrett]

> On Sep 10, 2020, at 6:21 AM, Thomas Schatzl <tschatzl at openjdk.java.net> wrote:
> 
> Hi,
> 
>  please review this re-application of JDK-8249676. The change passes two tier8 (plus tier1-7 which never were a problem)
>  runs w/o problems; the crashes I reported in the original review are gone after recent (other) changes.
> 
> Note that this PR contains two commits: the vanilla original patch and an update to fix merge errors with the recent
> IHOP changes.
> 
> Thanks,
>  Thomas
> 
> -------------
> 
> Commit messages:
> - Merge issues resolved
> - Original patch
> 
> Changes: https://git.openjdk.java.net/jdk/pull/108/files
> Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=108&range=00
>  Issue: https://bugs.openjdk.java.net/browse/JDK-8249676
>  Stats: 380 lines in 3 files changed: 178 ins; 63 del; 139 mod
>  Patch: https://git.openjdk.java.net/jdk/pull/108.diff
>  Fetch: git fetch https://git.openjdk.java.net/jdk pull/108/head:pull/108
> 
> PR: https://git.openjdk.java.net/jdk/pull/108

Mostly looks good.  A few minor nits, and a couple of questions.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1Policy.cpp 
 229 // The main reason is revising young length, with our without the GCLocker being

s/our/or/

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1Policy.cpp 
 253   // allocated more than that for reasons. In this case, use that.

s/for reasons/for various reasons/ ?

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1Policy.cpp 
 271     if (!next_gc_should_be_mixed(NULL, NULL)) {
 272       desired_eden_length_by_pause =
 273         calculate_desired_eden_length_by_pause(survivor_base_time_ms,
...
 276     } else {
 277       desired_eden_length_before_mixed =
 278         calculate_desired_eden_length_before_mixed(survivor_base_time_ms,
...
 282     // Above either sets desired_eden_length_by_pause or desired_eden_length_before_mixed,
 283     // the other is zero. Use the one that has been set below.
 284     uint desired_eden_length = MAX2(desired_eden_length_by_pause,
 285                                     desired_eden_length_before_mixed);

This would be simpler if there were
  calculate_desired_eden_length_by_pause
which conditionally called (based on next_gc_should_be_mixed) either
  calculate_desired_eden_length_before_young_only (new name)
  calculate_desired_eden_length_before_mixed

However, this would lose the logging distinction between the two. Is that
logging distinction important enough to justify uglifying the code?

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1Policy.cpp  
1116   uint max_length = target_young_length + expansion_region_num;
1117   assert(target_young_length <= max_length, "post-condition");

The assertion seems rather pointless, since we're dealing with unsigned
values.  But maybe the intent is to check for overflow, in which case the
assertion message should say so.

------------------------------------------------------------------------------ 
src/hotspot/share/gc/g1/g1Policy.cpp 
1114     assert(expansion_region_num == 0, "sanity");

[pre-existing] This assertion seems pointless.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1Policy.cpp  
1364   if (reclaimable_percent <= threshold) {

The two logging messages that follow are rather excessively long.

And if they were formatted differently it might make it more obvious they
are identical except for the parenthetical part of the message.  (Or at
least I *think* they are; not so easy to compare by eye across horizongal
scrolling or wrapping.)

(I looked at combining them; not sure it's worthwhile.)

------------------------------------------------------------------------------