8178095: Add GC stress test TestSystemGC

[Erik Helin]

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