Quantcast

RFR: 8177968: Add GC stress test TestGCLocker

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: 8177968: Add GC stress test TestGCLocker

Erik Helin-2
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/

Testing:
- JPRT (to ensure libTestGCLocker.c compiles on all platforms)
- make run-test TEST=hotspot/test/gc/stress/gclocker

Thanks,
Erik
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177968: Add GC stress test TestGCLocker

Per Liden
Hi Erik,

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 that this test provokes the wanted
situation in only ~20% of the GC. Being a stress test, I'd like to
propose that we remove the sleep() calls to have it provoke the
situation ~100% of the GCs.

--- 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);
          }
      }
  }


Also, I think the filler function doesn't do what it says as it only
writes to the first byte. 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);
  }


cheers,
Per

>
> Testing:
> - JPRT (to ensure libTestGCLocker.c compiles on all platforms)
> - make run-test TEST=hotspot/test/gc/stress/gclocker
>
> Thanks,
> Erik
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177968: Add GC stress test TestGCLocker

Per Liden
In reply to this post by Erik Helin-2
Hi Erik,

(Re-sending, as Thunderbird crashed on me and it seems that my first
reply never made it to the list)

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.

--- 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);
          }
      }
  }


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);
  }


Other than that, looks good!

cheers,
Per

> Testing:
> - JPRT (to ensure libTestGCLocker.c compiles on all platforms)
> - make run-test TEST=hotspot/test/gc/stress/gclocker
>
> Thanks,
> Erik
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177968: Add GC stress test TestGCLocker

Erik Helin-2
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177968: Add GC stress test TestGCLocker

Leonid Mesnik
Hi

Sorry for late response.

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.

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177968: Add GC stress test TestGCLocker

Per Liden
In reply to this post by Erik Helin-2
Hi Erik,

On 2017-04-18 11: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/

Thanks for fixing. Looks good!

/Per

>
> 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177968: Add GC stress test TestGCLocker

Erik Helin-2
On 04/18/2017 11:56 AM, Per Liden wrote:

> Hi Erik,
>
> On 2017-04-18 11: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/
>
> Thanks for fixing. Looks good!

Thanks!
Erik

> /Per
>
>>
>> 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177968: Add GC stress test TestGCLocker

Erik Helin-2
In reply to this post by Leonid Mesnik
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
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177968: Add GC stress test TestGCLocker

Erik Helin-2
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
>>>
>
Loading...