RFR (S): 8067868: Add GCOld as a JTreg test

[Bengt Rutisson]

Hi Kim,

Thanks for looking at this!

On 2014-12-18 22:25, Kim Barrett wrote:
> On Dec 18, 2014, at 7:45 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
>> Could I have a couple of reviews for this change to make GCOld a JTreg test?
>>
>> Root repo changes:
>> http://cr.openjdk.java.net/~brutisso/8067868/root-webrev.00/
>>
>> HotSpot repo changes:
>> http://cr.openjdk.java.net/~brutisso/8067868/hotspot-webrev.00/
>>
>> Bug report:
>> https://bugs.openjdk.java.net/browse/JDK-8067868
> ------------------------------------------------------------------------------
> test/stress/gc/TestGCOld.java
>
>    31  * @run main/othervm -Xmx384M -XX:+UseParallelGC TestGCOld 50 1 20 10 10000
>    32  * @run main/othervm -Xmx384M -XX:+UseParallelGC -XX:-UseParallelOldGC TestGCOld 50 1 20 10 10000
>
> I think line 31 contains the implicit assumption that
> -XX:+UseParallelOldGC is the default?  I think it would be better to
> make that explicit.

I think the normal way of selecting the ParallelGC is to use 
-XX:+UseParallelGC. This implies the ParallelOldGC. When we ask people 
to run the parallel collector I think we normally just tell them to run 
with -XX:+UseParallelGC. So, to me it is easier to read it as it is now. 
Adding more flags makes me have to look closer to see why they are there.

>
> And I' not sure what collector get's used for line 32.  Is it serial?

It is the ParallelScavenge young collector with the serial old collector.

>
> Maybe line 31 should just be -XX:+UseParallelOldGC (which implies
> -XX:+UseParallelGC).

As I described above this is backwards to how we normally handle it in 
my opinion.

> And line 32 should be replaced by a set of lines
> of -XX:+UseParallelGC -XX:+Use{oldspace gc compatible with parallel young}.

Not sure I understand this. ParallelScavenge can only be combined with 
the ParallelOldGC or the SerialGC and these are the two lines at 31 and 32.

> I didn't really look at the details of the test itself.  I assume it
> came from elsewhere, more or less unchanged except for the jtreg
> header comment?

Yes, that is right. As I wrote to Dima, I prefer not to change the test 
as part of this changeset.

>
> ------------------------------------------------------------------------------
> test/TEST.groups
>
>   444 hotspot_jprt = \
>   ...
>   451   :hotspot_gc_gcold \
>   ...
>
> I'm surprised a "stress" test is being added to the jprt set.
>
> ------------------------------------------------------------------------------
>

GCOld was always run in JPRT. I just moved it into the JTreg harness. I 
figured it is a kind of different test than the normal tests we have had 
there until now. That's why I called it stress. I'm planning to move one 
more test in there too - GCBasher.

Thanks,
Bengt