<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Bengt,<br>
<br>
Thanks for looking over the changes. Replies inline....<br>
<br>
<div class="moz-cite-prefix">On 3/21/2013 11:48 PM, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:514BFECC.8070206@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi John,<br>
<br>
Your changes look good to me.<br>
<br>
I think your motivation for removing the verification from
universe2_init() and init_globals() is fine. Actually I wonder
why they were there in the first place, but they do seem
intentionally put in there. However, I'm fine with removing
them.<br>
</div>
</blockquote>
<br>
I think they we're deliberately added - probably a very long time
ago. And Thomas' speculation that it's just to narrow the window in
case of an error sounds plausible. But since parallel scavenge
skipped the generations until the the first true GC - not sure how
useful it would be for PS. Likewise for G1 - in the default case we
skipped these extra verifications and in the first verification we
skipped part of the heap. <br>
<br>
<blockquote cite="mid:514BFECC.8070206@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
About the test. Great that you write a regression test for this!
:)<br>
<br>
The @summary says that the test uses
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
-XX:+VerifyDuringGC but the command line is actually using
-XX:+VerifyBeforeGC (which is correct, I think). Also, would it
make sense to have a separate test that specifies -XX:+UseG1GC
and checks the output that we expect to see?<br>
</div>
</blockquote>
<br>
Yeah. Good catch. It should be with VerifyBeforeGC. Must have had
marking on the brain.<br>
<br>
As for the test. I think we can check the output for "VerifyBefore"
for all the collectors. I'll change the test.<br>
<br>
<blockquote cite="mid:514BFECC.8070206@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
One question that is not strictly related to your change:<br>
<br>
The code to do the verification in Threads::create_vm() is:<br>
<br>
3449 if (VerifyBeforeGC &&<br>
3450 Universe::heap()->total_collections() >=
VerifyGCStartAt) {<br>
3451 Universe::heap()->prepare_for_verify();<br>
3452 Universe::verify(); // make sure we're starting with
a clean slate<br>
3453 }<br>
<br>
This is what it looked like before your change as well. But to
me this looks kind of odd. First, we re-use the flag
VerifyBeforeGC even though we are not about to do a GC. I can
live with that, but it is kind of strange. Then we have the
check against VerifyGCStartAt. By default this is 0 so we will
do the verification. But why do we do this check? There is no
chance that we have been able to do any GC yet, right? So,
checking against Universe::heap()->total_collections() seems
unnecessary. We should either check VerifyGCStartAt == 0 or not
include that flag at all (best choice in my opinion).<br>
</div>
</blockquote>
<br>
I think this was a case of cut-n-paste from the GC code. I agree
overriding the flag is strange - especially given that we have a
flag for VerifyOnExit (or something like that). But it a pattern
that we, in the GC team, recognize. :) I agree that checking against
total_collections() is bogus. It will be 0 and so we'll skip the
verification if VerifyGCStartAt is anything other than 0. I guess
two choices:<br>
<br>
1. Add new flag (or rename existing VerifyOnExit to
VerifyOnInitAndExit), or<br>
2. Use (VerifyBeforeGC && VerifyGCStartAt == 0)<br>
<br>
Cheers,<br>
<br>
JohnC<br>
</body>
</html>