RFR (S): 8067868: Add GCOld as a JTreg test
Hi everyone, 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 Thanks, Bengt
On Dec 18, 2014, at 7:45 AM, Bengt Rutisson <bengt.rutisson@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. And I' not sure what collector get's used for line 32. Is it serial? Maybe line 31 should just be -XX:+UseParallelOldGC (which implies -XX:+UseParallelGC). And line 32 should be replaced by a set of lines of -XX:+UseParallelGC -XX:+Use{oldspace gc compatible with parallel young}. 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? ------------------------------------------------------------------------------ test/TEST.groups 444 hotspot_jprt = \ ... 451 :hotspot_gc_gcold \ ... I'm surprised a "stress" test is being added to the jprt set. ------------------------------------------------------------------------------
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@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
On Dec 19, 2014, at 9:28 AM, Bengt Rutisson <bengt.rutisson@oracle.com> wrote:
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.
Oops, I was looking at old (jdk7) documentation; things are different in jdk8+. Sorry for the noise.
------------------------------------------------------------------------------ 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.
OK. Change looks good to me.
Hi Bengt, I didn't try to understand the logic in detail, I think this is not new test. What I noticed: - the code doesn't meet java coding style (indent should be 4) - checkTrees()method is declared, but all invocations are commented out. If we don't need the method we can remove it - this test never fails (only if OOM or crash). Is it expected? Thanks, Dima On 18.12.2014 15:45, Bengt Rutisson wrote:
Hi everyone,
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
Thanks, Bengt
Hi Dima, Thanks for looking at this. On 2014-12-19 14:36, Dmitry Fazunenko wrote:
Hi Bengt,
I didn't try to understand the logic in detail, I think this is not new test. What I noticed: - the code doesn't meet java coding style (indent should be 4) - checkTrees()method is declared, but all invocations are commented out. If we don't need the method we can remove it - this test never fails (only if OOM or crash). Is it expected?
Yes, just as you noted this is not a new test. I only moved it. I would prefer to not move it and change it at the same time. This review is just about moving the test into JTreg. Once it is there we can start cleaning it up. Thanks, Bengt
Thanks, Dima
On 18.12.2014 15:45, Bengt Rutisson wrote:
Hi everyone,
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
Thanks, Bengt
On 19.12.2014 17:15, Bengt Rutisson wrote:
Hi Dima,
Thanks for looking at this.
On 2014-12-19 14:36, Dmitry Fazunenko wrote:
Hi Bengt,
I didn't try to understand the logic in detail, I think this is not new test. What I noticed: - the code doesn't meet java coding style (indent should be 4) - checkTrees()method is declared, but all invocations are commented out. If we don't need the method we can remove it - this test never fails (only if OOM or crash). Is it expected?
Yes, just as you noted this is not a new test. I only moved it. I would prefer to not move it and change it at the same time. This review is just about moving the test into JTreg. Once it is there we can start cleaning it up.
If you believe that this test checks something (or could be updated to check something) - the fix is fine by me. Thanks, Dima
Thanks, Bengt
Thanks, Dima
On 18.12.2014 15:45, Bengt Rutisson wrote:
Hi everyone,
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
Thanks, Bengt
On 2014-12-19 15:35, Dmitry Fazunenko wrote:
On 19.12.2014 17:15, Bengt Rutisson wrote:
Hi Dima,
Thanks for looking at this.
On 2014-12-19 14:36, Dmitry Fazunenko wrote:
Hi Bengt,
I didn't try to understand the logic in detail, I think this is not new test. What I noticed: - the code doesn't meet java coding style (indent should be 4) - checkTrees()method is declared, but all invocations are commented out. If we don't need the method we can remove it - this test never fails (only if OOM or crash). Is it expected?
Yes, just as you noted this is not a new test. I only moved it. I would prefer to not move it and change it at the same time. This review is just about moving the test into JTreg. Once it is there we can start cleaning it up.
If you believe that this test checks something (or could be updated to check something) - the fix is fine by me.
Yes, the test has been part of our testing in JPRT for many, many years. And it does find issues. So, I think it is a valuable test to keep. But it can definitely be improved. Bengt
Thanks, Dima
Thanks, Bengt
Thanks, Dima
On 18.12.2014 15:45, Bengt Rutisson wrote:
Hi everyone,
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
Thanks, Bengt
On 2014-12-18, Bengt Rutisson wrote:
Hi everyone,
Could I have a couple of reviews for this change to make GCOld a JTreg test?
Looks good, Reviewed. Thanks, Erik
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
Thanks, Bengt
On 2015-01-07 16:05, Erik Helin wrote:
On 2014-12-18, Bengt Rutisson wrote:
Hi everyone,
Could I have a couple of reviews for this change to make GCOld a JTreg test? Looks good, Reviewed.
Thanks for the reviews Dima, Kim and Erik! Bengt
Thanks, Erik
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
Thanks, Bengt
participants (4)
-
Bengt Rutisson
-
Dmitry Fazunenko
-
Erik Helin
-
Kim Barrett