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