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

[Leonid Mesnik]

Hi

> On Aug 27, 2018, at 1:05 AM, Per Liden <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 <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.

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.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180827/9986ee10/attachment.htm>