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

[Stefan Johansson]

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)
   */
---

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