<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><pre><b style="font-family: Times; font-size: medium;">src/share/vm/runtime/sharedRuntime.cpp:</b></pre><pre>&nbsp;<font color="blue"><b>+     bool contains_all_checks = (StubRoutines::code2() != NULL) || !VerifyAdapterCalls;</b></font></pre><div><div>Could you move the&nbsp;<b style="color: blue;">!VerifyAdapterCalls </b>check to here:</div><div><pre><font color="blue"><b>+     if (contains_all_checks) {</b></font></pre></div><div>And I still don’t know what “checks” means in this context. &nbsp;Anyway, this looks good.</div><div><br></div><div>On Oct 24, 2013, at 5:30 AM, Albert Noll &lt;<a href="mailto:albert.noll@oracle.com">albert.noll@oracle.com</a>&gt; wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">
  
    <meta content="text/html; charset=windows-1252" http-equiv="Content-Type">
  
  <div bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">Hi John,<br>
      <br>
      thank you for looking at this. You are right, the changes to
      free_entry are not correct when -XX:VerifyAdapterSharing is <br>
      specified.<br>
      <br>
      An alternative way to solve the problem is to add entrys only if
      the entry contains all checks and <br>
      -XX:+VerifyAdapterCalls is specified. With this version, we also
      do not need the _contains_all_checks <br>
      field in AdapterHandlerEntry.<br>
      <br>
      What do you think about this version?<br>
      <meta http-equiv="content-type" content="text/html;
        charset=windows-1252">
      <a href="http://cr.openjdk.java.net/%7Eanoll/8026478/webrev.04/">http://cr.openjdk.java.net/~anoll/8026478/webrev.04/</a><br>
      Best,<br>
      Albert<br>
      <br>
      <br>
      On 23.10.2013 18:59, John Rose wrote:<br>
    </div>
    <blockquote cite="mid:420AA490-51F4-459C-982A-606F885FD575@oracle.com" type="cite">
      <meta http-equiv="Content-Type" content="text/html;
        charset=windows-1252">
      <div>
        <div>I have a question: &nbsp;The amended&nbsp;free_entry function
          indirectly unlinks the selected 'entry', but may also unlink
          other entries with the same hash and fingerprint. &nbsp;Isn't this
          working against the use for free_entry, where a pre-existing
          shared_entry is to be favored and returned? &nbsp;Won't the
          shared_entry be unlinked also? &nbsp;That would seem to lead to&nbsp;a
          storage leak.</div>
        <div><br>
        </div>
        <div>(Minor style nit: &nbsp;You code the hash comparison and key
          comparison as nested ifs; they would be just as readable
          connected by an '&amp;&amp;', and with less nesting.)</div>
        <div><br>
        </div>
        <div>— John</div>
        <div><br>
        </div>
        <div>On Oct 23, 2013, at 6:49 AM, Albert Noll &lt;<a moz-do-not-send="true" href="mailto:albert.noll@oracle.com">albert.noll@oracle.com</a>&gt;
          wrote:</div>
        <br class="Apple-interchange-newline">
        <blockquote type="cite">
          <meta content="text/html; charset=windows-1252" http-equiv="Content-Type">
          <div bgcolor="#FFFFFF" text="#000000">
            <div class="moz-cite-prefix">Hi,<br>
              <br>
              unfortunately, the patch was not correct, as revealed by
              jprt.<br>
              The problem was that entries into the hashmap are not
              removed<br>
              correctly. <br>
              <br>
              This version fixes the problem. The patch is currently in
              jprt.<br>
              So far, everything looks good.<br>
              <br>
              Many thanks for another round. Here is the updated webrev:<br>
              <meta http-equiv="content-type" content="text/html;
                charset=windows-1252">
              <a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Eanoll/8026478/webrev.03/">http://cr.openjdk.java.net/~anoll/8026478/webrev.03/</a><br>
              <br>
              Best,<br>
              Albert<br>
              <br>
              <br>
              On 16.10.2013 19:43, Christian Thalinger wrote:<br>
            </div>
            <blockquote cite="mid:34569632-9CA9-4205-BDED-09177AB0EFB0@oracle.com" type="cite">
              <meta http-equiv="Content-Type" content="text/html;
                charset=windows-1252">
              It would be nice to have a comment on this guy:
              <div>
                <pre><font color="blue"><b>+   bool    _contains_all_checks;</b></font></pre>
                <div>
                  <div>No need for another webrev if you decide to add
                    the comment.</div>
                  <div><br>
                  </div>
                  <div>Looks good.</div>
                  <div><br>
                  </div>
                  <div>On Oct 15, 2013, at 11:02 PM, Albert Noll &lt;<a moz-do-not-send="true" href="mailto:albert.noll@oracle.com">albert.noll@oracle.com</a>&gt;

                    wrote:</div>
                  <br class="Apple-interchange-newline">
                  <blockquote type="cite">
                    <meta content="text/html; charset=windows-1252" http-equiv="Content-Type">
                    <div bgcolor="#FFFFFF" text="#000000">
                      <div class="moz-cite-prefix">Forgot to adjust the
                        comment. This version should be fine.<br>
                        <br>
                        <meta http-equiv="content-type" content="text/html; charset=windows-1252">
                        <a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Eanoll/8026478/webrev.02/">http://cr.openjdk.java.net/~anoll/8026478/webrev.02/</a><br>
                        <br>
                        Albert<br>
                        <br>
                        On 15.10.2013 21:03, Vladimir Kozlov wrote:<br>
                      </div>
                      <blockquote cite="mid:525D9194.3070403@oracle.com" type="cite">Albert, <br>
                        <br>
                        contains_all_stubs name is incorrect. It should
                        be contains_all_checks. Adapter code does not
                        have stubs in it. <br>
                        <br>
                        I think the next check should be: <br>
                        <br>
                        bool contains_all_checks =
                        (StubRoutines::code2() != NULL) ||
                        !VerifyAdapterCalls; <br>
                        <br>
                        because code does not change when
                        VerifyAdapterCalls is false. <br>
                        Then you don't need to check VerifyAdapterCalls
                        in next condition: <br>
                        <br>
                        +&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; if ((VerifyAdapterCalls ||
                        VerifyAdapterSharing) &amp;&amp;
                        !entry-&gt;contains_all_checks()) { <br>
                        <br>
                        And we need to regenerate adapter regardless
                        VerifyAdapterSharing because, as you said,
                        VerifyAdapterCalls potentially reports a false
                        error if check for code2 is not generated. <br>
                        <br>
                        So the condition should be simple "if
                        (!entry-&gt;contains_all_checks())". <br>
                        <br>
                        Could you leave next lines unchanged (use 2
                        lines for arguments)?: <br>
                        !&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
                        buffer.insts()-&gt;initialize_shared_locs((relocInfo*)buffer_locs,
                        <br>
                        <br>
                        sizeof(buffer_locs)/sizeof(relocInfo)); <br>
                        &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; MacroAssembler _masm(&amp;buffer); <br>
                        <br>
                        Cleanup is fine. <br>
                        <br>
                        Thanks, <br>
                        Vladimir <br>
                        <br>
                        On 10/15/13 4:14 AM, Albert Noll wrote: <br>
                        <blockquote type="cite">Hi, <br>
                          <br>
                          could I have reviews for this small patch? <br>
                          <br>
                          Bug: <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8026478">https://bugs.openjdk.java.net/browse/JDK-8026478</a>
                          <br>
                          webrev: <a moz-do-not-send="true" class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Eanoll/8026478/webrev.00/">http://cr.openjdk.java.net/~anoll/8026478/webrev.00/</a>
                          <br>
                          <a moz-do-not-send="true" class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Eanoll/8026478/webrev.00/">&lt;http://cr.openjdk.java.net/%7Eanoll/8026478/webrev.00/&gt;</a>
                          <br>
                          <br>
                          Problem: This flag is broken. The reason is
                          that adapters <br>
                          (gen_i2c_adapter()) that are generated during
                          VM startup do not include <br>
                          the range for StubRoutines::code2() if
                          -XX:+VerifyAdapterCalls is <br>
                          specified. StubRoutines::code2() is only
                          allocated later during startup; <br>
                          consequently, code2() is NULL. As a result,
                          the size of the generated <br>
                          adapter as well as the code itself does not
                          mactch. <br>
                          <br>
                          Solution: Defer check (VerifyAdapterSharing)
                          until VM is initialzied. <br>
                          Note that this patch also fixes potential
                          issues with <br>
                          -XX:+VerifyAdapterCalls, which is a dignostic
                          flag. The patch also <br>
                          removes some unused field of
                          VerifyAdapterSharing. <br>
                          <br>
                          <br>
                          Many thanks in advance, <br>
                          Albert <br>
                          ** <br>
                        </blockquote>
                      </blockquote>
                      <br>
                    </div>
                  </blockquote>
                </div>
                <br>
              </div>
            </blockquote>
            <br>
          </div>
        </blockquote>
      </div>
      <br>
    </blockquote>
    <br>
  </div>

</blockquote></div><br></body></html>