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
Thu Dec 20 21:26:23 UTC 2018 

Hi Kishor,

On 12/20/18 12:18 PM, 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
Both webrev.16 and 17 look okay.

1. Regarding bitmap treatment, I agree with Stefan. i.e. go as-is for 
jdk12. It is better than the preliminary version at least.
2. Please fix the typo below:
src/hotspot/share/gc/g1/heterogeneousHeapRegionManager.cpp
   53   // This allows regions to be un-committed while 
concurrent-marking threads are accesing the bitmap concurrently.
- accesing -> accessing

Thanks,
Sangheon


>
> 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.
>
> 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_committed
>>> () <= 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