8209920: runtime/logging/RedefineClasses.java fail with OOME with ZGC

[Per Liden]

Hi,

On 08/27/2018 12:24 PM, Leonid Mesnik wrote:
> Hi
> 
>> On Aug 27, 2018, at 1:05 AM, Per Liden <per.liden at oracle.com 
>> <mailto:per.liden at oracle.com>> wrote:
>>
>> Hi Leonid,
>>
>> On 08/24/2018 10:44 PM, Leonid Mesnik wrote:
>>> Hi
>>> Could you please review following fix. I think that fix could be 
>>> considered as trivial. So I would like to push it once I get Review 
>>> to avoid failures in tier3.
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8209920
>>> diff:
>>> diff -r 9d01ad46daef 
>>> test/hotspot/jtreg/runtime/logging/RedefineClasses.java
>>> --- a/test/hotspot/jtreg/runtime/logging/RedefineClasses.java   Fri 
>>> Aug 24 15:49:21 2018 -0400
>>> +++ b/test/hotspot/jtreg/runtime/logging/RedefineClasses.java   Fri 
>>> Aug 24 13:23:17 2018 -0700
>>> @@ -26,6 +26,7 @@
>>> * @bug 8197901 8209758
>>> * @summary Redefine classes with enabling logging to verify 
>>> Klass:external_name() during GC.
>>> * @comment This test is simplified version of 
>>> runtime/RedefineTests/RedefineRunningMethods.java.
>>> + * @requires vm.opt.final.ClassUnloading
>>> * @library /test/lib
>>> * @modules java.compiler
>>> *          java.instrument
>>
>> I don't think this addresses the root cause. The test fails because 
>> we're running out of heap, not because we didn't unload classes. In 
>> other words, the test would still fail if you were running ZGC with 
>> class unloading enabled.
>>
>> The problem is that the test is given 256m of heap, and the obj[] 
>> array keeps 20 * 10M alive. This will actually occupy 7*32M = 224M of 
>> medium ZPages on the heap, which leaves 32M for the rest. However, 
>> ZGC's heap reserve is 32M + 2M * nworkers, which means we're out of 
>> memory.
>>
>> So, instead of adding the vm.opt.final.ClassUnloading attribute, I'd 
>> suggest we reduce the size of the obj[] array to 10 instead of 20. 
>> That would leave enough heap available for the test to pass.
> Thanks for good explanation of how memory is used by ZGC. I agree that 
> such fix makes test to pass with ZGC
> 
> However test doesn't provoke crash described in original bug when 
> classes are not unloaded. At least it always pass with ZGC enabled.  It 
> pass even fix from bug
> https://bugs.openjdk.java.net/browse/JDK-8197901
> is not applied. So I've thought that it is not needed to be run.
> 
> The number of attempts to make GC was just used in original 
> test runtime/RedefineTests/RedefineRunningMethods.java. It is important 
> to run GC more then 10 times to provoke original crash. While test 
> does't need to occupy ~200M of live memory to provoke crash. It is 
> enough to produce smaller amount of garbage for this, so it would be 
> better just to reduce size of byte[] array.

I see.

> 
> If you think that it makes sense to run test with ZGC right now then see 
> fix below.  It might be a still good stress test for combination of 
> logging/GC/class[un]loading for any GC.
> I verified that test pass with ZGC and still provokes original crash 
> when run with G1.

Since it's a quick test I think it's worth running on all GCs, even if 
ZGC doesn't currently do class unloading.

> 
> diff -r 9d01ad46daef test/hotspot/jtreg/runtime/logging/RedefineClasses.java
> --- a/test/hotspot/jtreg/runtime/logging/RedefineClasses.java   Fri Aug 
> 24 15:49:21 2018 -0400
> +++ b/test/hotspot/jtreg/runtime/logging/RedefineClasses.java   Mon Aug 
> 27 03:01:25 2018 -0700
> @@ -53,7 +53,7 @@
>           RedefineClassHelper.redefineClass(RedefineClasses_B.class, newB);
>           RedefineClasses_B.test();
>           for (int i = 0; i < 20 ; i++) {
> -            obj[i] = new byte[10 * 1024 * 1024];
> +            obj[i] = new byte[1024 * 1024];
>               System.gc();
>           }
>       }
> 
> 
> What do you think this fix?

Yep, looks good.

cheers,
Per

>>
>> diff --git a/test/hotspot/jtreg/runtime/logging/RedefineClasses.java 
>> b/test/hotspot/jtreg/runtime/logging/RedefineClasses.java
>> --- a/test/hotspot/jtreg/runtime/logging/RedefineClasses.java
>> +++ b/test/hotspot/jtreg/runtime/logging/RedefineClasses.java
>> @@ -42,7 +42,7 @@
>> }
>>
>> public class RedefineClasses {
>> -    static Object[] obj = new Object[20];
>> +    static Object[] obj = new Object[10];
>>     public static String newB =
>>             "class RedefineClasses_B {" +
>>             "    public static void test() { " +
>> @@ -52,7 +52,7 @@
>>     public static void main(String[] args) throws Exception {
>>         RedefineClassHelper.redefineClass(RedefineClasses_B.class, newB);
>>         RedefineClasses_B.test();
>> -        for (int i = 0; i < 20 ; i++) {
>> +        for (int i = 0; i < obj.length; i++) {
>>             obj[i] = new byte[10 * 1024 * 1024];
>>             System.gc();
>>         }
>>
>> On the other hand, it's not clear to me what the alloc+System.gc() 
>> for-loop is trying to accomplish and why it is needed. I can remove 
>> the whole for-loop and the test passes anyway, regardless of which GC 
>> i pick. Am I missing something?
> The idea is to provoke crash described in the 
> https://bugs.openjdk.java.net/browse/JDK-8197901
> It happens during GC with enabled logging. This is why GC is actually 
> needed.
> 
> Leonid
>>
>> cheers,
>> Per
>