RFR (M): 8212657: Implementation of JDK-8204089 Promptly Return Unused Committed Memory from G1 [Was: RFR (M): 8212657: Implementation of JDK-8204089 Timely Reduce Unused Committed Memory]

[sangheon.kim at oracle.com]

Hi Thomas,

On 12/7/18 11:07 AM, Thomas Schatzl wrote:
> Hi Sangheon,
>
> On Fri, 2018-12-07 at 07:04 -0800, sangheon.kim at oracle.com wrote:
>> Hi Thomas,
>>
>> On 12/6/18 5:54 AM, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>> On Tue, 2018-12-04 at 10:32 +0100, Stefan Johansson wrote:
>>>> On 2018-12-03 15:30, Thomas Schatzl wrote:
>>>>> Hi Sangheon,
>>>>>
>>> [...]
>>>>> All fixed.
>>>>>
>>>>> http://cr.openjdk.java.net/~tschatzl/8212657/webrev.1_to_2/
>>>>> (diff)
>>>>> http://cr.openjdk.java.net/~tschatzl/8212657/webrev.2/ (full)
>>>> Still good,
>>>> StefanJ
>>>     while doing testing of other changes (where I included this
>>> change) I
>>> found that very rarely the test fails.
>>>
>>> This happens if the test machine is loaded, and additional heap
>>> resizing happens due to gctimeratio being too high while we are
>>> waiting
>>> for a periodic gc.
>>>
>>> E.g.
>>>
>>> - <create garbage>
>>> - some gcs happen
>>> - concurrent mark starts
>>> - g1 finds out that gc time ratio is too high -> expands the heap
>>> by
>>> itself
>>> - concurrent mark finds that we recently expanded too much, shrinks
>>> the
>>> heap
>>> - periodic gc finds no more opportunity to shrink -> test fails
>>>
>>> I trimmed the test to never do any GC (not allocating anything) by
>>> setting appropriate startup options but still find opportunity to
>>> shrink with periodic gcs.
>>>
>>> This makes the test much shorter in code and execution time, and
>>> much
>>> more robust. :)
>>>
>>> http://cr.openjdk.java.net/~tschatzl/8212657/webrev.2_to_3/ (diff)
>>> http://cr.openjdk.java.net/~tschatzl/8212657/webrev.3/ (full)
>> Looks good!
>    thanks for your review - but unfortunately this test fails when it is
> run during high-load on the machine. The problem is that it happens
> that (periodic) GCs may have occurred before getting the initial heap
> size reading, which will shrink the heap.
>
> I had to fix this by a) disallowing running the test when there is
> significant delay for getting into the main method, e.g. using -Xcomp,
> and b) check that after reading the initial heap sizes there were no
> GCs that could have possibly shrinked the heap.
>
> If so, just bail out with some explanation.
>
> http://cr.openjdk.java.net/~tschatzl/8212657/webrev.3_to_4/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8212657/webrev.4/ (full)
Still looks good.

Thanks,
Sangheon


>
> :(
>
> This passes hs-tier1-5 now, also did some local testing forcing the
> issue.
>
> Thanks,
>    Thomas
>
>