<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi,<br>
<br>
Bengt:<br>
<br>
Right, that should be enough, thanks.<br>
<br>
Mikael:<br>
<br>
Can I use you as a reviewer for this latest version as well?<br>
<br>
Regards,<br>
Andreas<br>
<br>
On 2014-05-22 13:02, Bengt Rutisson wrote:<br>
</div>
<blockquote cite="mid:537DD93E.1010303@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi Andreas,<br>
<br>
This is a HotSpot change and for JDK7 HotSpot was developed in
the hsx project. I am a Reviewer in the hsx project (<a
moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://openjdk.java.net/census#brutisso">http://openjdk.java.net/census#brutisso</a>)
isn't that enough to review this change?<br>
<br>
Anyway, last webrev looks good. Thanks for fixing the test! <br>
<br>
Bengt<br>
<br>
On 5/22/14 10:43 AM, Andreas Eriksson wrote:<br>
</div>
<blockquote cite="mid:537DB8B1.5080905@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi,<br>
<br>
Adding jdk7u-dev.<br>
<br>
Could I have a jdk7u Reviewer look at this as well please?
This is a jdk7 only fix.<br>
(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.)<br>
<br>
Description:<br>
Due to the marking cleanup reclaiming empty regions, and
having stale references a crash can occur when doing a heap
dump.<br>
The code tries to do an is_klass check on the object, which
crashes the VM.<br>
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.<br>
<br>
For more information, and a more in-depth analysis, please see
the jira bug.<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eaeriksso/8038925/webrev.03/">http://cr.openjdk.java.net/~aeriksso/8038925/webrev.03/</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8038925">https://bugs.openjdk.java.net/browse/JDK-8038925</a><br>
<br>
Regards,<br>
Andreas<br>
<br>
On 2014-05-22 10:14, Andreas Eriksson wrote:<br>
</div>
<blockquote cite="mid:537DB1F2.6020406@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">OK, I'll remove it.<br>
<br>
Thanks,<br>
Andreas<br>
<br>
On 2014-05-22 10:02, Bengt Rutisson wrote:<br>
</div>
<blockquote cite="mid:537DAEFC.2000508@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
<br>
Hi Andreas,<br>
<br>
On 5/21/14 2:05 PM, Andreas Eriksson wrote:<br>
</div>
<blockquote cite="mid:537C9695.5010902@oracle.com"
type="cite">A new webrev is up: <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eaeriksso/8038925/webrev.02/">http://cr.openjdk.java.net/~aeriksso/8038925/webrev.02/</a>
<br>
<br>
The test now verifies that no full GC has been done after
doing the heap dump. <br>
I also modified the test to not run if it for some reason
is not using G1. <br>
</blockquote>
<br>
Thanks for the update, Andreas.<br>
<br>
The test looks good except for the @run tag.<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
@run main/othervm -Xms512m -Xmx1024m -XX:+UseG1GC
-XX:+ExplicitGCInvokesConcurrent TestG1ConcurrentGCHeapDump<br>
<br>
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.<br>
<br>
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.<br>
<br>
Bengt<br>
<br>
<blockquote cite="mid:537C9695.5010902@oracle.com"
type="cite"> <br>
Regards, <br>
Andreas <br>
<br>
On 2014-05-21 12:31, Bengt Rutisson wrote: <br>
<blockquote type="cite">On 5/21/14 12:12 PM, Andreas
Eriksson wrote: <br>
<blockquote type="cite">Hi Bengt, thanks for looking at
this. <br>
<br>
I agree that verifying that no full GC has happened
would be good. <br>
One thing I see as problematic though, is what to do
if a full GC has happened. <br>
Should I make the test fail? Or is there some way to
signal that the test was inconclusive / couldn't
finish? <br>
</blockquote>
<br>
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. <br>
<br>
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. <br>
<br>
(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.) <br>
<br>
Thanks, <br>
Bengt <br>
<br>
<blockquote type="cite"> <br>
Regards, <br>
Andreas <br>
<br>
On 2014-05-21 11:55, Bengt Rutisson wrote: <br>
<blockquote type="cite"> <br>
Hi Andreas, <br>
<br>
The fix looks good. <br>
<br>
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. <br>
<br>
I'm fine with leaving the test as it is. <br>
<br>
Thanks, <br>
Bengt <br>
<br>
<br>
On 5/20/14 4:26 PM, Andreas Eriksson wrote: <br>
<blockquote type="cite">Hi, <br>
<br>
Could I have a review for this G1 jdk7 only fix
please? <br>
(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.) <br>
<br>
Description: <br>
Due to the marking cleanup reclaiming empty
regions, and having stale references a crash can
occur when doing a heap dump. <br>
The code tries to do an is_klass check on the
object, which crashes the VM. <br>
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. <br>
<br>
For more information, and a more in-depth
analysis, please see the jira bug. <br>
<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eaeriksso/8038925/webrev.01/">http://cr.openjdk.java.net/~aeriksso/8038925/webrev.01/</a>
<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8038925">https://bugs.openjdk.java.net/browse/JDK-8038925</a>
<br>
<br>
Regards, <br>
Andreas <br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>