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

[Thomas Schatzl]

Hi Kim,

   thanks for your review.

On 13.09.20 03:41, Kim Barrett wrote:
>> 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/ ?

Fixed.

> 
> ------------------------------------------------------------------------------
> 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?

Refactored as suggested. I think the information loss is minimal as we 
can easily infer from which path the value in the log message comes from.

> 
> ------------------------------------------------------------------------------
> 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.

Fixed.

> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1Policy.cpp
> 1114     assert(expansion_region_num == 0, "sanity");
> 
> [pre-existing] This assertion seems pointless.

Removed.

> 
> ------------------------------------------------------------------------------
> 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.)
> 

Fixed them a bit, but did not combine the messages. From starting on 
this I saw that it made the code more complicated than worth.

I rebased the changes in the PR for easier testing (tier1,2 running). 
The webrev did not work before and does not now 
(https://bugs.openjdk.java.net/browse/SKARA-595), so it's probably best 
again to look at the diff in github at

https://github.com/openjdk/jdk/pull/108/commits/ff64ae374001706958062750b5144d2c61417a69

Thanks,
   Thomas