8178095: Add GC stress test TestSystemGC

[Erik Helin]

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