8178095: Add GC stress test TestSystemGC
Dmitry Fazunenko
dmitry.fazunenko at oracle.com
Wed Apr 5 13:11:06 UTC 2017
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
>>
More information about the hotspot-gc-dev
mailing list