Quantcast

8178095: Add GC stress test TestSystemGC

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

8178095: Add GC stress test TestSystemGC

Erik Helin-2
Hi all,

this patch adds a small stress test called TestSystemGC. The test
stresses a full GC implementation by allocating objects of different
life times while concurrently calling System.gc() repeatedly.

Enhancement:
https://bugs.openjdk.java.net/browse/JDK-8178095

Patch:
http://cr.openjdk.java.net/~ehelin/8178095/00/

Testing:
- make run-test TEST=hotspot/test/gc/stress/systemgc

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

Re: 8178095: Add GC stress test TestSystemGC

Dmitry Fazunenko
Hi Erik,

Overall the fix looks good to me.
Some minor questions/comments:

1. Have you tested your code with -XX:-UseCompressedOpps ?
     (you create a lot of objects and limit the heap by 512m)

2. Would you mind renaming TestSystemGC to SystemGCTest or similar. Just
to distinguish jtreg tests from libraries.

3. Adding a couple of sentences explaining the idea of TestSystemGC
would help.

4. You run the same method twice:

     public static void main(String[] args) {
         populateLongLived();
         runAllPhases();
         runAllPhases();
     }

Is it intentionally? If yes, would you add a small comment to the second
run (to avoid possible confusion)

5. Optional: you can move the sleep() method from ThreadUtils  to
TestSystemGC (to reduce the number of classes)

Thanks,
Dima


On 05.04.2017 10:14, Erik Helin wrote:

> Hi all,
>
> this patch adds a small stress test called TestSystemGC. The test
> stresses a full GC implementation by allocating objects of different
> life times while concurrently calling System.gc() repeatedly.
>
> Enhancement:
> https://bugs.openjdk.java.net/browse/JDK-8178095
>
> Patch:
> http://cr.openjdk.java.net/~ehelin/8178095/00/
>
> Testing:
> - make run-test TEST=hotspot/test/gc/stress/systemgc
>
> Thanks,
> Erik

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

Re: 8178095: Add GC stress test TestSystemGC

Erik Helin-2
Hey Dima,

thanks for having a look! Please see a new version at:
- inc: http://cr.openjdk.java.net/~ehelin/8177967/00-01/
- full: http://cr.openjdk.java.net/~ehelin/8177967/01/

On 04/05/2017 10:43 AM, Dmitry Fazunenko wrote:
> Hi Erik,
>
> Overall the fix looks good to me.
> Some minor questions/comments:
>
> 1. Have you tested your code with -XX:-UseCompressedOpps ?
>     (you create a lot of objects and limit the heap by 512m)

Well, it is meant to be run according to the @run tags. If a developer
runs with different flags, they are on their own :) But yeah, I took it
for a spin and it works (at least on my machine).

> 2. Would you mind renaming TestSystemGC to SystemGCTest or similar. Just
> to distinguish jtreg tests from libraries.

Hmm, all the other stress tests already use the Test*.java convention,
like TestGCBasher and TestGCOld. Some of the other tests do prefix with
TestStress*.java, but not all.

I would prefer to keep the name the file TestSystemGC.java, to follow
the existing convention. Since the 'stress' directory should already
indicate that this is a stress test, I would prefer to not prefix with
'TestStress'.

> 3. Adding a couple of sentences explaining the idea of TestSystemGC
> would help.

I tried to do that as part of the @summary tag in each jtreg test :) I
added small comment at the top of TestSystemGC as well.

> 4. You run the same method twice:
>
>     public static void main(String[] args) {
>         populateLongLived();
>         runAllPhases();
>         runAllPhases();
>     }
>
> Is it intentionally? If yes, would you add a small comment to the second
> run (to avoid possible confusion)

Yep, intentionally, I added a comment.

> 5. Optional: you can move the sleep() method from ThreadUtils  to
> TestSystemGC (to reduce the number of classes)

I think I keep it in ThreadUtils for now, thanks,

Erik

> Thanks,
> Dima
>
>
> On 05.04.2017 10:14, Erik Helin wrote:
>> Hi all,
>>
>> this patch adds a small stress test called TestSystemGC. The test
>> stresses a full GC implementation by allocating objects of different
>> life times while concurrently calling System.gc() repeatedly.
>>
>> Enhancement:
>> https://bugs.openjdk.java.net/browse/JDK-8178095
>>
>> Patch:
>> http://cr.openjdk.java.net/~ehelin/8178095/00/
>>
>> Testing:
>> - make run-test TEST=hotspot/test/gc/stress/systemgc
>>
>> Thanks,
>> Erik
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 8178095: Add GC stress test TestSystemGC

Dmitry Fazunenko
Erik,

New version looks good!
Some comments inline.

Thanks,
Dima

On 05.04.2017 15:40, Erik Helin wrote:

> Hey Dima,
>
> thanks for having a look! Please see a new version at:
> - inc: http://cr.openjdk.java.net/~ehelin/8177967/00-01/
> - full: http://cr.openjdk.java.net/~ehelin/8177967/01/
>
> On 04/05/2017 10:43 AM, Dmitry Fazunenko wrote:
>> Hi Erik,
>>
>> Overall the fix looks good to me.
>> Some minor questions/comments:
>>
>> 1. Have you tested your code with -XX:-UseCompressedOpps ?
>>     (you create a lot of objects and limit the heap by 512m)
>
> Well, it is meant to be run according to the @run tags. If a developer
> runs with different flags, they are on their own :) But yeah, I took
> it for a spin and it works (at least on my machine).

After integration the test become a part of regular execution, you know
) It might start failing in nightly...
Yes, running on the local machine should be enough.

>
>> 2. Would you mind renaming TestSystemGC to SystemGCTest or similar. Just
>> to distinguish jtreg tests from libraries.
>
> Hmm, all the other stress tests already use the Test*.java convention,
> like TestGCBasher and TestGCOld. Some of the other tests do prefix
> with TestStress*.java, but not all.
For me it's very convenient when you can say:
   #> jtreg Test*
If some of Test* do not have @test you will error from jtreg.

>
> I would prefer to keep the name the file TestSystemGC.java, to follow
> the existing convention. Since the 'stress' directory should already
> indicate that this is a stress test, I would prefer to not prefix with
> 'TestStress'.

Yes, you are right, there are a lot cases which do not meet this naming
convention, so it's unfair to request it.

>
>
>> 3. Adding a couple of sentences explaining the idea of TestSystemGC
>> would help.
>
> I tried to do that as part of the @summary tag in each jtreg test :) I
> added small comment at the top of TestSystemGC as well.

ok. that's enough.

>
>> 4. You run the same method twice:
>>
>>     public static void main(String[] args) {
>>         populateLongLived();
>>         runAllPhases();
>>         runAllPhases();
>>     }
>>
>> Is it intentionally? If yes, would you add a small comment to the second
>> run (to avoid possible confusion)
>
> Yep, intentionally, I added a comment.
good.

>
>> 5. Optional: you can move the sleep() method from ThreadUtils  to
>> TestSystemGC (to reduce the number of classes)
>
> I think I keep it in ThreadUtils for now, thanks,

ok.


Thanks,
Dima

>
> Erik
>
>> Thanks,
>> Dima
>>
>>
>> On 05.04.2017 10:14, Erik Helin wrote:
>>> Hi all,
>>>
>>> this patch adds a small stress test called TestSystemGC. The test
>>> stresses a full GC implementation by allocating objects of different
>>> life times while concurrently calling System.gc() repeatedly.
>>>
>>> Enhancement:
>>> https://bugs.openjdk.java.net/browse/JDK-8178095
>>>
>>> Patch:
>>> http://cr.openjdk.java.net/~ehelin/8178095/00/
>>>
>>> Testing:
>>> - make run-test TEST=hotspot/test/gc/stress/systemgc
>>>
>>> Thanks,
>>> Erik
>>

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

Re: 8178095: Add GC stress test TestSystemGC

Erik Helin-2
On 04/05/2017 03:11 PM, Dmitry Fazunenko wrote:
> Erik,
>
> New version looks good!

Thanks, and thanks for reviewing!

>>> 1. Have you tested your code with -XX:-UseCompressedOpps ?
>>>     (you create a lot of objects and limit the heap by 512m)
>>
>> Well, it is meant to be run according to the @run tags. If a developer
>> runs with different flags, they are on their own :) But yeah, I took
>> it for a spin and it works (at least on my machine).
>
> After integration the test become a part of regular execution, you know
> ) It might start failing in nightly...
> Yes, running on the local machine should be enough.

Ok, good.

>>> 2. Would you mind renaming TestSystemGC to SystemGCTest or similar. Just
>>> to distinguish jtreg tests from libraries.
>>
>> Hmm, all the other stress tests already use the Test*.java convention,
>> like TestGCBasher and TestGCOld. Some of the other tests do prefix
>> with TestStress*.java, but not all.
> For me it's very convenient when you can say:
>   #> jtreg Test*
> If some of Test* do not have @test you will error from jtreg.

Ah ok, then I get why you asked :) Given that all the tests are in a
directory called hotspot/test/gc/stress/systemgc, I just pass the
directory to jtreg (does not result in any errors)


>> I would prefer to keep the name the file TestSystemGC.java, to follow
>> the existing convention. Since the 'stress' directory should already
>> indicate that this is a stress test, I would prefer to not prefix with
>> 'TestStress'.
>
> Yes, you are right, there are a lot cases which do not meet this naming
> convention, so it's unfair to request it.

Ok, thanks.

>>> 3. Adding a couple of sentences explaining the idea of TestSystemGC
>>> would help.
>>
>> I tried to do that as part of the @summary tag in each jtreg test :) I
>> added small comment at the top of TestSystemGC as well.
>
> ok. that's enough.

Thanks :)

>>> 4. You run the same method twice:
>>>
>>>     public static void main(String[] args) {
>>>         populateLongLived();
>>>         runAllPhases();
>>>         runAllPhases();
>>>     }
>>>
>>> Is it intentionally? If yes, would you add a small comment to the second
>>> run (to avoid possible confusion)
>>
>> Yep, intentionally, I added a comment.
> good.
>
>>
>>> 5. Optional: you can move the sleep() method from ThreadUtils  to
>>> TestSystemGC (to reduce the number of classes)
>>
>> I think I keep it in ThreadUtils for now, thanks,
>
> ok.

Again, thanks Dima for reviewing!
Erik

> Thanks,
> Dima
>
>>
>> Erik
>>
>>> Thanks,
>>> Dima
>>>
>>>
>>> On 05.04.2017 10:14, Erik Helin wrote:
>>>> Hi all,
>>>>
>>>> this patch adds a small stress test called TestSystemGC. The test
>>>> stresses a full GC implementation by allocating objects of different
>>>> life times while concurrently calling System.gc() repeatedly.
>>>>
>>>> Enhancement:
>>>> https://bugs.openjdk.java.net/browse/JDK-8178095
>>>>
>>>> Patch:
>>>> http://cr.openjdk.java.net/~ehelin/8178095/00/
>>>>
>>>> Testing:
>>>> - make run-test TEST=hotspot/test/gc/stress/systemgc
>>>>
>>>> Thanks,
>>>> Erik
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 8178095: Add GC stress test TestSystemGC

Mikael Gerdin
Hi Erik,

Looks fine to me.
/Mikael

On 2017-04-07 14:04, Erik Helin wrote:

> On 04/05/2017 03:11 PM, Dmitry Fazunenko wrote:
>> Erik,
>>
>> New version looks good!
>
> Thanks, and thanks for reviewing!
>
>>>> 1. Have you tested your code with -XX:-UseCompressedOpps ?
>>>>     (you create a lot of objects and limit the heap by 512m)
>>>
>>> Well, it is meant to be run according to the @run tags. If a developer
>>> runs with different flags, they are on their own :) But yeah, I took
>>> it for a spin and it works (at least on my machine).
>>
>> After integration the test become a part of regular execution, you know
>> ) It might start failing in nightly...
>> Yes, running on the local machine should be enough.
>
> Ok, good.
>
>>>> 2. Would you mind renaming TestSystemGC to SystemGCTest or similar.
>>>> Just
>>>> to distinguish jtreg tests from libraries.
>>>
>>> Hmm, all the other stress tests already use the Test*.java convention,
>>> like TestGCBasher and TestGCOld. Some of the other tests do prefix
>>> with TestStress*.java, but not all.
>> For me it's very convenient when you can say:
>>   #> jtreg Test*
>> If some of Test* do not have @test you will error from jtreg.
>
> Ah ok, then I get why you asked :) Given that all the tests are in a
> directory called hotspot/test/gc/stress/systemgc, I just pass the
> directory to jtreg (does not result in any errors)
>
>
>>> I would prefer to keep the name the file TestSystemGC.java, to follow
>>> the existing convention. Since the 'stress' directory should already
>>> indicate that this is a stress test, I would prefer to not prefix with
>>> 'TestStress'.
>>
>> Yes, you are right, there are a lot cases which do not meet this naming
>> convention, so it's unfair to request it.
>
> Ok, thanks.
>
>>>> 3. Adding a couple of sentences explaining the idea of TestSystemGC
>>>> would help.
>>>
>>> I tried to do that as part of the @summary tag in each jtreg test :) I
>>> added small comment at the top of TestSystemGC as well.
>>
>> ok. that's enough.
>
> Thanks :)
>
>>>> 4. You run the same method twice:
>>>>
>>>>     public static void main(String[] args) {
>>>>         populateLongLived();
>>>>         runAllPhases();
>>>>         runAllPhases();
>>>>     }
>>>>
>>>> Is it intentionally? If yes, would you add a small comment to the
>>>> second
>>>> run (to avoid possible confusion)
>>>
>>> Yep, intentionally, I added a comment.
>> good.
>>
>>>
>>>> 5. Optional: you can move the sleep() method from ThreadUtils  to
>>>> TestSystemGC (to reduce the number of classes)
>>>
>>> I think I keep it in ThreadUtils for now, thanks,
>>
>> ok.
>
> Again, thanks Dima for reviewing!
> Erik
>
>> Thanks,
>> Dima
>>
>>>
>>> Erik
>>>
>>>> Thanks,
>>>> Dima
>>>>
>>>>
>>>> On 05.04.2017 10:14, Erik Helin wrote:
>>>>> Hi all,
>>>>>
>>>>> this patch adds a small stress test called TestSystemGC. The test
>>>>> stresses a full GC implementation by allocating objects of different
>>>>> life times while concurrently calling System.gc() repeatedly.
>>>>>
>>>>> Enhancement:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8178095
>>>>>
>>>>> Patch:
>>>>> http://cr.openjdk.java.net/~ehelin/8178095/00/
>>>>>
>>>>> Testing:
>>>>> - make run-test TEST=hotspot/test/gc/stress/systemgc
>>>>>
>>>>> Thanks,
>>>>> Erik
>>>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 8178095: Add GC stress test TestSystemGC

Erik Helin-2
On 04/07/2017 02:13 PM, Mikael Gerdin wrote:
> Hi Erik,
>
> Looks fine to me.
> /Mikael

Thanks Mikael for taking your time to review!
Erik

> On 2017-04-07 14:04, Erik Helin wrote:
>> On 04/05/2017 03:11 PM, Dmitry Fazunenko wrote:
>>> Erik,
>>>
>>> New version looks good!
>>
>> Thanks, and thanks for reviewing!
>>
>>>>> 1. Have you tested your code with -XX:-UseCompressedOpps ?
>>>>>     (you create a lot of objects and limit the heap by 512m)
>>>>
>>>> Well, it is meant to be run according to the @run tags. If a developer
>>>> runs with different flags, they are on their own :) But yeah, I took
>>>> it for a spin and it works (at least on my machine).
>>>
>>> After integration the test become a part of regular execution, you know
>>> ) It might start failing in nightly...
>>> Yes, running on the local machine should be enough.
>>
>> Ok, good.
>>
>>>>> 2. Would you mind renaming TestSystemGC to SystemGCTest or similar.
>>>>> Just
>>>>> to distinguish jtreg tests from libraries.
>>>>
>>>> Hmm, all the other stress tests already use the Test*.java convention,
>>>> like TestGCBasher and TestGCOld. Some of the other tests do prefix
>>>> with TestStress*.java, but not all.
>>> For me it's very convenient when you can say:
>>>   #> jtreg Test*
>>> If some of Test* do not have @test you will error from jtreg.
>>
>> Ah ok, then I get why you asked :) Given that all the tests are in a
>> directory called hotspot/test/gc/stress/systemgc, I just pass the
>> directory to jtreg (does not result in any errors)
>>
>>
>>>> I would prefer to keep the name the file TestSystemGC.java, to follow
>>>> the existing convention. Since the 'stress' directory should already
>>>> indicate that this is a stress test, I would prefer to not prefix with
>>>> 'TestStress'.
>>>
>>> Yes, you are right, there are a lot cases which do not meet this naming
>>> convention, so it's unfair to request it.
>>
>> Ok, thanks.
>>
>>>>> 3. Adding a couple of sentences explaining the idea of TestSystemGC
>>>>> would help.
>>>>
>>>> I tried to do that as part of the @summary tag in each jtreg test :) I
>>>> added small comment at the top of TestSystemGC as well.
>>>
>>> ok. that's enough.
>>
>> Thanks :)
>>
>>>>> 4. You run the same method twice:
>>>>>
>>>>>     public static void main(String[] args) {
>>>>>         populateLongLived();
>>>>>         runAllPhases();
>>>>>         runAllPhases();
>>>>>     }
>>>>>
>>>>> Is it intentionally? If yes, would you add a small comment to the
>>>>> second
>>>>> run (to avoid possible confusion)
>>>>
>>>> Yep, intentionally, I added a comment.
>>> good.
>>>
>>>>
>>>>> 5. Optional: you can move the sleep() method from ThreadUtils  to
>>>>> TestSystemGC (to reduce the number of classes)
>>>>
>>>> I think I keep it in ThreadUtils for now, thanks,
>>>
>>> ok.
>>
>> Again, thanks Dima for reviewing!
>> Erik
>>
>>> Thanks,
>>> Dima
>>>
>>>>
>>>> Erik
>>>>
>>>>> Thanks,
>>>>> Dima
>>>>>
>>>>>
>>>>> On 05.04.2017 10:14, Erik Helin wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> this patch adds a small stress test called TestSystemGC. The test
>>>>>> stresses a full GC implementation by allocating objects of different
>>>>>> life times while concurrently calling System.gc() repeatedly.
>>>>>>
>>>>>> Enhancement:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8178095
>>>>>>
>>>>>> Patch:
>>>>>> http://cr.openjdk.java.net/~ehelin/8178095/00/
>>>>>>
>>>>>> Testing:
>>>>>> - make run-test TEST=hotspot/test/gc/stress/systemgc
>>>>>>
>>>>>> Thanks,
>>>>>> Erik
>>>>>
>>>
Loading...