RFR: 8177968: Add GC stress test TestGCLocker

[Leonid Mesnik]

Yes, test looks good. No separate review for adding key is needed.

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