<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix"><br>
      Hi John,<br>
      <br>
      Some comments inline...<br>
      <br>
      On 3/22/13 7:19 PM, John Cuthbertson wrote:<br>
    </div>
    <blockquote cite="mid:514CA0C3.3080306@oracle.com" type="cite">
      <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
      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>
    </blockquote>
    <br>
    Sound reasonable. Thanks for the extra explanation.<br>
    <br>
    <blockquote cite="mid:514CA0C3.3080306@oracle.com" type="cite"> <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>
    </blockquote>
    <br>
    Great!<br>
    <br>
    <blockquote cite="mid:514CA0C3.3080306@oracle.com" type="cite"> <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>
    </blockquote>
    <br>
    Right. I would prefer 1. but I'm fine with 2. Maybe 2. is more
    reasonable change to do for this fix. Perhaps 1. should be its own
    change.<br>
    <br>
    Either way is fine with me.<br>
    <br>
    Thanks,<br>
    Bengt<br>
    <br>
    <br>
    <blockquote cite="mid:514CA0C3.3080306@oracle.com" type="cite"> <br>
      Cheers,<br>
      <br>
      JohnC<br>
    </blockquote>
    <br>
  </body>
</html>