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