RFR: 8177968: Add GC stress test TestGCLocker

[Erik Helin]

On 04/18/2017 12:18 PM, Leonid Mesnik wrote:
> Yes, test looks good. No separate review for adding key is needed.

Thanks!
Erik

> Leonid
>
>
> On 18.04.2017 13:05, Erik Helin wrote:
>> On 04/18/2017 11:30 AM, Leonid Mesnik wrote:
>>> Hi
>>
>> Hey Leonid!
>>
>>> Sorry for late response.
>>
>> No worries, thanks for reviewing :)
>>
>>> Could you please add tag ' @key stress' to the tests. It helps to
>>> exclude them using keywords when only fast tests are going to be run.
>>> I am not sure if it is used right now in the test execution for gc
>>> tests. However it is a good practice and all other gc stress tests are
>>> marked.
>>
>> Sure, will do that before I push. Do you think the test looks good
>> otherwise?
>>
>> Thanks,
>> Erik
>>
>>> Leonid
>>>
>>>
>>> On 18.04.2017 12:22, Erik Helin wrote:
>>>> On 04/12/2017 08:05 AM, Per Liden wrote:
>>>>> Hi Erik,
>>>>>
>>>>> (Re-sending, as Thunderbird crashed on me and it seems that my first
>>>>> reply never made it to the list)
>>>>
>>>> Hey Per,
>>>>
>>>> thanks for reviewing! Please see new version at
>>>> - inc: http://cr.openjdk.java.net/~ehelin/8177968/00-01/
>>>> - full: http://cr.openjdk.java.net/~ehelin/8177968/01/
>>>>
>>>> and comments inline:
>>>>
>>>>> On 2017-04-07 17:48, Erik Helin wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> this patch adds the stress test "TestGCLocker". The test repeatedly
>>>>>> calls GCLocker::lock_critical/unlock_critical (via the JNI functions
>>>>>> GetPrimitiveArrayCritical/ReleasePrimitiveArrayCritical) while
>>>>>> concurrently filling up the old gen. Thea idea is to stress the
>>>>>> GCLocker
>>>>>> implementation by quickly entering/leaving critical JNI sections
>>>>>> while
>>>>>> simultaneously allocating objects to fill up the heap in order to
>>>>>> provoke a GC.
>>>>>>
>>>>>> Enhancement:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8177968
>>>>>>
>>>>>> Patch:
>>>>>> http://cr.openjdk.java.net/~ehelin/8177968/00/
>>>>>
>>>>> I did some test runs and it seems the test only provokes the wanted
>>>>> situation in ~20% of the GCs (on my machine at least). Being a stress
>>>>> test, I'd like to propose that we remove the sleep() calls to have it
>>>>> provoke the situation in ~100% of the GCs.
>>>>
>>>> Thanks for taking the code for a spin!
>>>>
>>>>> --- a/test/gc/stress/gclocker/TestGCLocker.java
>>>>> +++ b/test/gc/stress/gclocker/TestGCLocker.java
>>>>> @@ -162,7 +153,6 @@
>>>>>
>>>>>          while (!shouldExit()) {
>>>>>              load();
>>>>> -            ThreadUtils.sleep(100);
>>>>>          }
>>>>>      }
>>>>>  }
>>>>> @@ -175,7 +165,6 @@
>>>>>          byte[] array = new byte[1024 * 1024];
>>>>>          while (!shouldExit()) {
>>>>>              fillWithRandomValues(array);
>>>>> -            ThreadUtils.sleep(10);
>>>>>          }
>>>>>      }
>>>>>  }
>>>>
>>>> Great suggestion, will update the patch.
>>>>
>>>>> Also, the filler function only writes to the first byte, which looks
>>>>> like a bug. Simple fix:
>>>>>
>>>>> --- a/test/gc/stress/gclocker/libTestGCLocker.c
>>>>> +++ b/test/gc/stress/gclocker/libTestGCLocker.c
>>>>> @@ -29,7 +29,7 @@
>>>>>    jbyte* p = (*env)->GetPrimitiveArrayCritical(env, arr, NULL);
>>>>>    jsize i;
>>>>>    for (i = 0; i < size; i++) {
>>>>> -      *p = i % 128;
>>>>> +    p[i] = i % 128;
>>>>>    }
>>>>>    (*env)->ReleasePrimitiveArrayCritical(env, arr, p, 0);
>>>>>  }
>>>>
>>>> Heh, thanks for noticing this :) Reminds me of:
>>>> http://dilbert.com/strip/2001-10-25
>>>>
>>>>> Other than that, looks good!
>>>>
>>>> Thanks!
>>>> Erik
>>>>
>>>>> cheers,
>>>>> Per
>>>>>
>>>>>> Testing:
>>>>>> - JPRT (to ensure libTestGCLocker.c compiles on all platforms)
>>>>>> - make run-test TEST=hotspot/test/gc/stress/gclocker
>>>>>>
>>>>>> Thanks,
>>>>>> Erik
>>>
>