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

[Stefan Johansson]

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