Great, thanks.
/Andreas
On 2014-05-23 12:37, Mikael Gerdin wrote:
On Thursday 22 May 2014 13.39.13 Andreas Eriksson wrote:
Hi,
Bengt:
Right, that should be enough, thanks.
Mikael:
Can I use you as a reviewer for this latest version as well?
Yes, this looks fine.
/Mikael
Regards,
Andreas
On 2014-05-22 13:02, Bengt Rutisson wrote:
Hi Andreas,
This is a HotSpot change and for JDK7 HotSpot was developed in the hsx
project. I am a Reviewer in the hsx project
(http://openjdk.java.net/census#brutisso) isn't that enough to review
this change?
Anyway, last webrev looks good. Thanks for fixing the test!
Bengt
On 5/22/14 10:43 AM, Andreas Eriksson wrote:
Hi,
Adding jdk7u-dev.
Could I have a jdk7u Reviewer look at this as well please? This is a
jdk7 only fix.
(There is a related problem in jdk8 and jdk9, where an assert can
fail because of this problem. I have logged another bug for this.)
Description:
Due to the marking cleanup reclaiming empty regions, and having stale
references a crash can occur when doing a heap dump.
The code tries to do an is_klass check on the object, which crashes
the VM.
The fix is to add an is_perm check before doing the check, since
is_perm will do a bounds check on the oop and if it's in the perm gen
we know it's safe to look at it since G1 only ever does full
compactions of the perm gen.
For more information, and a more in-depth analysis, please see the
jira bug.
http://cr.openjdk.java.net/~aeriksso/8038925/webrev.03/
https://bugs.openjdk.java.net/browse/JDK-8038925
Regards,
Andreas
On 2014-05-22 10:14, Andreas Eriksson wrote:
OK, I'll remove it.
Thanks,
Andreas
On 2014-05-22 10:02, Bengt Rutisson wrote:
Hi Andreas,
On 5/21/14 2:05 PM, Andreas Eriksson wrote:
A new webrev is up:
http://cr.openjdk.java.net/~aeriksso/8038925/webrev.02/
The test now verifies that no full GC has been done after doing
the heap dump.
I also modified the test to not run if it for some reason is not
using G1.
Thanks for the update, Andreas.
The test looks good except for the @run tag.
@run main/othervm -Xms512m -Xmx1024m -XX:+UseG1GC
-XX:+ExplicitGCInvokesConcurrent TestG1ConcurrentGCHeapDump
The problem is that more GC flags will be added when the test is
run in nightly testing. Some GC flags will conflict with UseG1GC.
On the other hand at some point UseG1GC will be one of the flags
that is added.
So, I think what you need to do is just remove "-XX:+UseG1GC" from
the @run tag. Then your test will report log "skipped" when it is
run in the nightly testing except for the nightly testing done with
G1 where it will actually do something.
Bengt
Regards,
Andreas
On 2014-05-21 12:31, Bengt Rutisson wrote:
On 5/21/14 12:12 PM, Andreas Eriksson wrote:
Hi Bengt, thanks for looking at this.
I agree that verifying that no full GC has happened would be good.
One thing I see as problematic though, is what to do if a full
GC has happened.
Should I make the test fail? Or is there some way to signal that
the test was inconclusive / couldn't finish?
Personally I would prefer to make the test fail. JTreg only has
two states, PASS or FAIL, and I think that if we allow it to pass
we might not notice if there is some change that makes the test
always get a full GC and then never actually testing anything.
I don't think it will fail intermittently by getting full GCs. I
think the test is pretty stable. But I think it would be good to
have a way of noticing if it stops testing what it is supposed to
test.
(By the way, I would really like a "SKIPPED" state in JTreg but I
haven't had any luck pushing for that. I think it could be useful
for other things as well. There is for example no good reason for
your test to be run 4 times each night with the exact same
binary. But that is what happens since we can't say that we
should skip this test if we run with any other GC than G1.)
Thanks,
Bengt
Regards,
Andreas
On 2014-05-21 11:55, Bengt Rutisson wrote:
Hi Andreas,
The fix looks good.
One comment about the test. It does not verify that no full GC
happens. The way the test is set up I guess that should not
happen and I am not sure it is worth the effort to add a check
for it. Just wanted to mention it if you want to make test more
resilient to future changes in the JVM that for some reason can
trigger a full GC for this test.
I'm fine with leaving the test as it is.
Thanks,
Bengt
On 5/20/14 4:26 PM, Andreas Eriksson wrote:
Hi,
Could I have a review for this G1 jdk7 only fix please?
(There is a related problem in jdk8 and jdk9, where an assert
can fail because of this problem. I have logged another bug
for this.)
Description:
Due to the marking cleanup reclaiming empty regions, and
having stale references a crash can occur when doing a heap dump.
The code tries to do an is_klass check on the object, which
crashes the VM.
The fix is to add an is_perm check before doing the check,
since is_perm will do a bounds check on the oop and if it's in
the perm gen we know it's safe to look at it since G1 only
ever does full compactions of the perm gen.
For more information, and a more in-depth analysis, please see
the jira bug.
http://cr.openjdk.java.net/~aeriksso/8038925/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8038925
Regards,
Andreas
