RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC

sangheon.kim at oracle.com sangheon.kim at oracle.com
Fri Dec 21 02:58:51 UTC 2018 

Hi Kishor,

On 12/20/18 4:58 PM, Kharbas, Kishor wrote:
> Hi,
> I have new webrev at http://cr.openjdk.java.net/~kkharbas/8211425/webrev.17_to_18/
> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.18/
Webrev.18 looks good.
Thank you for addressing my requests.
I will update you after some tests finished.

Thanks,
Sangheon


>
> This update corrects the typo and add releasing of temporary nv-dimm file.
>
> Tested with tier1 jtreg tests.
>
> Thanks
> Kishor
>
>> -----Original Message-----
>> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
>> Sent: Thursday, December 20, 2018 2:10 PM
>> To: Kharbas, Kishor <kishor.kharbas at intel.com>; sangheon.kim at oracle.com
>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>; 'hotspot-gc-
>> dev at openjdk.java.net' <hotspot-gc-dev at openjdk.java.net>; 'Hotspot dev
>> runtime' <hotspot-runtime-dev at openjdk.java.net>
>> Subject: Re: RFR(M): 8211425: Allocation of old generation of java heap on
>> alternate memory devices - G1 GC
>>
>> Hi Kishor,
>>
>> On 2018-12-20 21:18, Kharbas, Kishor wrote:
>>> Thanks Stefan,
>>>
>>> I was able to reproduce and fix the issue.
>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.17
>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.16_to_17
>>>
>>> On my system (2S Skylake server, with 28 cores/56 threads per socket), I do
>> not see new failures when new flag is enabled.
>>> About the fix for latest issue:
>>> When handling evacuation failure, I overlooked the scenario when
>> G1AllocationFailure is for multi-region humongous object.
>>> satisfy_failed_allocation() for multi-region humongous object should also
>> fail when we have non-zero borrowed_regions.
>>> The fix in the webrev solves the problem and the failed test is now
>> successful.
>> Fix looks good, I will push it through our testing environment for verification.
>>
>> Thanks,
>> Stefan
>>> As you suggested, I will add an RFE for adding a better abstraction for CM
>> prev/next bitmap.
>>> Thanks
>>> Kishor
>>>
>>>> -----Original Message-----
>>>> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
>>>> Sent: Thursday, December 20, 2018 6:43 AM
>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
>>>> sangheon.kim at oracle.com
>>>> Cc: Viswanathan, Sandhya <sandhya.viswanathan at intel.com>;
>>>> 'hotspot-gc- dev at openjdk.java.net' <hotspot-gc-dev at openjdk.java.net>;
>>>> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>
>>>> Subject: Re: RFR(M): 8211425: Allocation of old generation of java
>>>> heap on alternate memory devices - G1 GC
>>>>
>>>> Hi,
>>>>
>>>> On 2018-12-20 15:21, Stefan Johansson wrote:
>>>>> Hi,
>>>>>
>>>>> On 2018-12-20 08:47, Kharbas, Kishor wrote:
>>>>>> Hi Stefan, Sangheon,
>>>>>>
>>>>>> I have a new webrev -
>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.16
>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.15_to_16
>>>>> I think the solution to never have the bitmaps uncommitted is
>>>>> acceptable but I would like us to clean it up going forward. Please
>>>>> create a RFE for adding a better abstraction to avoid having to
>>>>> explicitly call commit_and_set_special. Basically we know when we
>>>>> are creating the mapper for the bitmaps that they need to be pinned
>>>>> and we should be able to pass that in as a argument to the constructor.
>>>>>
>>>>> Otherwise I think this looks ok.
>>>>>
>>>>>> 1. It incorporates comment about FormatBuffer and has some
>>>>>> alignment changes.
>>>>> Thanks.
>>>>>
>>>>>> 2. Provides fix for the tests which fail when feature is turned on.
>>>>>> Comments in heterogeneousHeapRegionManager.cpp:52 and
>>>>>> heterogeneousHeapRegionManager.cpp:472 describe them.
>>>>> I'm still seeing some tests asserting when running with the feature
>>>>> turned on, they all hit the same assert:
>>>>> Crash: Internal Error
>>>>>
>> ...heterogeneousHeapRegionManager.cpp...assert(total_regions_committe
>>>> d
>>>>> () <= max_expandable_length()) failed: must be
>>>>>
>>>>> Tests:
>>>>>
>> vmTestbase/vm/gc/concurrent/lp30yp25rp30mr70st0/TestDescription.java
>> vmTestbase/vm/gc/concurrent/lp30yp25rp30mr0st300/TestDescription.java
>> vmTestbase/vm/gc/compact/Compact_InternedStrings_NonbranchyTree/Te
>>>> stDe
>>>>> scription.java
>>>>>
>>>>> vmTestbase/gc/lock/jniref/jnireflock01/TestDescription.java
>>>>> vmTestbase/gc/lock/jni/jnilock001/TestDescription.java
>>>>> vmTestbase/gc/lock/jni/jnilock003/TestDescription.java
>>>>>
>>>>> I have had a hard time reproducing those locally but managed to get
>>>>> the first to fail by setting it to use 12 threads. This has to be
>>>>> added in the test file, below is a diff I used to make the test fail
>>>>> more
>>>>> frequently:
>>>>> ---
>>>>> diff --git
>>>>>
>> a/test/hotspot/jtreg/vmTestbase/vm/gc/concurrent/lp30yp25rp0mr30st300
>>>> /
>>>>> TestDescription.java
>>>>>
>> b/test/hotspot/jtreg/vmTestbase/vm/gc/concurrent/lp30yp25rp0mr30st300
>>>> /
>>>>> TestDescription.java
>>>>>
>>>>> ---
>>>>>
>> a/test/hotspot/jtreg/vmTestbase/vm/gc/concurrent/lp30yp25rp0mr30st300
>>>> /
>>>>> TestDescription.java
>>>>>
>>>>> +++
>>>>>
>> b/test/hotspot/jtreg/vmTestbase/vm/gc/concurrent/lp30yp25rp0mr30st300
>>>> /
>>>>> TestDescription.java
>>>>>
>>>>> @@ -41,6 +41,7 @@
>>>>>      *      -mr 30
>>>>>      *      -st 300
>>>>>      *      -ms high
>>>>> + *      -t 12
>>>>>      *      -gp circularList(high)
>>>>>      *      -gp1 nonbranchyTree(low)
>>>>>      */
>>>>> ---
>>>> Realized I pasted the wrong diff, this is the test I'm making fail
>>>> with the above
>>>> change:
>>>>
>> vmTestbase/vm/gc/concurrent/lp30yp25rp30mr70st0/TestDescription.java
>>>> Cheers,
>>>> Stefan
>>>>> Please look into what the cause for this is.
>>>>>
>>>>> Cheers,
>>>>> Stefan
>>>>>
>>>>>> I am able to successfully re-run the failed tests. I am re-run the
>>>>>> whole test suite, will provide status in the morning PST.
>>>>>>
>>>>>> Thanks
>>>>>> Kishor
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
>>>>>>> Sent: Monday, December 17, 2018 3:06 AM
>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
>>>>>>> sangheon.kim at oracle.com
>>>>>>> Cc: 'hotspot-gc-dev at openjdk.java.net'
>>>>>>> <hotspot-gc-dev at openjdk.java.net>;
>>>>>>> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
>>>>>>> Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>>>>>> Subject: Re: RFR(M): 8211425: Allocation of old generation of java
>>>>>>> heap on alternate memory devices - G1 GC
>>>>>>>
>>>>>>> Hi Kishor,
>>>>>>>
>>>>>>> Thanks for addressing my second to last comment :) Unfortunately
>>>>>>> the current patch does not build. Instead of using a temp string
>>>>>>> and os::snprintf you should use FromatBuffer::append with the
>>>>>>> format string.
>>>>>>> I've uploaded a patch fixing this at:
>>>>>>> http://cr.openjdk.java.net/~sjohanss/8211425/Final-Comment-G1/
>>>>>>>
>>>>>>> Please incorporate this into your changeset.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Stefan
>>>>>>>
>>>>>>> On 2018-12-15 03:04, Kharbas, Kishor wrote:
>>>>>>>> Thanks for the review,
>>>>>>>>
>>>>>>>> New webrev at -
>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.15
>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.14_to_15
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> Kishor
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Stefan Johansson [mailto:stefan.johansson at oracle.com]
>>>>>>>>> Sent: Friday, December 14, 2018 2:40 AM
>>>>>>>>> To: Kharbas, Kishor <kishor.kharbas at intel.com>;
>>>>>>>>> sangheon.kim at oracle.com
>>>>>>>>> Cc: 'hotspot-gc-dev at openjdk.java.net'
>>>>>>>>> <hotspot-gc-dev at openjdk.java.net>;
>>>>>>>>> 'Hotspot dev runtime' <hotspot-runtime-dev at openjdk.java.net>;
>>>>>>>>> Viswanathan, Sandhya <sandhya.viswanathan at intel.com>
>>>>>>>>> Subject: Re: RFR(M): 8211425: Allocation of old generation of
>>>>>>>>> java heap on alternate memory devices - G1 GC
>>>>>>>>>
>>>>>>>>> Hi Kishor,
>>>>>>>>>
>>>>>>>>> I have one (maybe last) comment on this and it is basically the
>>>>>>>>> same thing as I commented on for Parallel. I think you should
>>>>>>>>> use FormatBuffer instead of a raw char buffer in the sizing code.
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>> Stefan
>>>>>>>>>
>>>>>>>>> On 2018-12-14 08:54, Kharbas, Kishor wrote:
>>>>>>>>>> Hi Sangheon, Stefan,
>>>>>>>>>>
>>>>>>>>>> I have updated webrev at -
>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.14/
>>>>>>>>>> http://cr.openjdk.java.net/~kkharbas/8211425/webrev.13_to_14
>>>>>>>>>>
>>>>>>>>>> This webrev works on the latest comments and has some changes
>>>>>>>>>> in young
>>>>>>>>> generation sizing logs messages.
>>>>>>>>>> I am re-running jtreg tests with this patch.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Kishor
>>>>>>>>>>

More information about the hotspot-gc-dev mailing list