<html>
  <head>
    <meta content="text/html; charset=ISO-8859-1"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">Hi Frederik,<br>
      <br>
      Will review this tomorrow.<br>
      Sorry for the latency, was busy with other things.<br>
      <br>
      In general, I do not like the idea to skip the synchronization.<br>
      Also, it is important to keep the information from JVMTI
      consistent.<br>
      The data returned by the
      <meta http-equiv="content-type" content="text/html;
        charset=ISO-8859-1">
      getLoadedClasses and
      <meta http-equiv="content-type" content="text/html;
        charset=ISO-8859-1">
      getClassLoaderClasses should match.<br>
      We also need to make sure it is consistent with other info the
      JVMTI provides.<br>
      This requirement is not for improvement or simplification.<br>
      <br>
      But more tomorrow...<br>
      <br>
      Thanks,<br>
      Serguei<br>
      <br>
      <br>
      On 10/2/13 2:28 AM, Fredrik Arvidsson wrote:<br>
    </div>
    <blockquote cite="mid:524BE733.2070205@oracle.com" type="cite">
      <meta content="text/html; charset=ISO-8859-1"
        http-equiv="Content-Type">
      Hi and thanks for all your comments.<br>
      <br>
      I have simplified the code in <b>instanceKlass.cpp</b> like
      suggested and here is a new webrev:<br>
      <a moz-do-not-send="true"
        href="http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.01/">http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/</a><br>
      <br>
      Regarding the need to synchronize the access to
      ClassLoaderDataGraph I have come to the conclusion that it should
      be safe to call this without any additional synchronization since
      newly loaded and added classes are appended to the end of its
      'list' of classes. This would mean that the call could 'miss' the
      classes added during the collection phase, but this is not a big
      issue. I hope that my conclusion is correct.<br>
      <br>
      I believe that the
      JvmtiGetLoadedClasses::getClassLoaderClasses(...) method is to be
      left alone this time since I got the impression that only
      SystemDictionary 'knows' about initiating class loaders.<br>
      <br>
      There is plenty of room for improvement and simplification of much
      of the code in this area, but I feel that it must wait until
      another time. The task to re-factor and simplify would quickly
      grow out of hands I'm afraid :)<br>
      <br>
      Cheers<br>
      /Fredrik<br>
      <br>
      <div class="moz-cite-prefix">On 2013-10-01 09:34, <a
          moz-do-not-send="true" class="moz-txt-link-abbreviated"
          href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
        wrote:<br>
      </div>
      <blockquote cite="mid:524A7B07.1000300@oracle.com" type="cite">
        <meta content="text/html; charset=ISO-8859-1"
          http-equiv="Content-Type">
        <div class="moz-cite-prefix">Hi Frederik,<br>
          <br>
          <br>
          Thank you for jumping on this issue!<br>
          <br>
          <br>
          <meta http-equiv="content-type" content="text/html;
            charset=ISO-8859-1">
          <b>src/share/vm/oops/instanceKlass.cpp</b><br>
          <meta http-equiv="content-type" content="text/html;
            charset=ISO-8859-1">
          <pre><span class="new">2333   int src_index = 0;</span>
2334   while (src_index < src_length) {
2335     dest[dest_index++] = src[src_index++];
2336   }
<span class="new">2337 </span>
<span class="new">2338   // If we have a hash, append it</span>
<span class="new">2339   if(hash_len > 0) {</span>
<span class="new">2340     int hash_index = 0;</span>
<span class="new">2341     while (hash_index < hash_len) {</span>
<span class="new">2342       dest[dest_index++] = hash_buf[hash_index++];</span>
<span class="new">2343     }</span>
<span class="new">2344   }

The above can be simplified a little bit:
   // Add the actual class name
   for (int src_index = 0; src_index < src_length; ) {
     dest[dest_index++] = src[src_index++];
   }
   // If we have a hash, append it
   for (int hash_index = 0; hash_index < hash_len; ) {
     dest[dest_index++] = hash_buf[hash_index++];
   }

The condition </span><span class="new">if(hash_len > 0)</span> is covered by the loop itself.

Style-related comments:
 - <meta http-equiv="content-type" content="text/html; charset=ISO-8859-1"><span class="new">wrong indent at the lines: 2316</span>-2319
 - need a space after the 'if' at the lines: 2315, 2339


<meta http-equiv="content-type" content="text/html; charset=ISO-8859-1"><b>src/share/vm/prims/jvmtiGetLoadedClasses.cpp

</b>The copyright comment must be up-to-date.
The comments at the lines 35-38, 258-260 are not valid anymore.


> In this review I still have an outstanding unanswered question about
>     the need to synchronize the access to the:
    
<span class="new" style="color: blue; font-weight: normal;">> ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);
<font color="#000000">> and</font>
</span><span class="new" style="color: blue; font-weight: normal;">> ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);</span>

This kind of walking is usually done at safepoint in a VM_Operation.
You will find many examples in the jvmtiEnv.cpp, for instance, VM_GetAllStackTraces.
The suggestion from Mikael is good too.

Should this fix include the getClassLoaderClasses() as well?
I'm still trying to realize the impact and consistency of this fix. :(

What tests are you going to run for verification?

Thanks,
Serguei
<b>
</b>On 9/30/13 4:45 AM, Fredrik Arvidsson wrote:
</pre>
        </div>
        <blockquote cite="mid:5249643F.3030209@oracle.com" type="cite">
          <meta http-equiv="content-type" content="text/html;
            charset=ISO-8859-1">
          Hi<br>
          <br>
          Please help me to review the changes below:<br>
          <br>
          Jira case: <a moz-do-not-send="true"
            href="https://bugs.openjdk.java.net/browse/JDK-8024423">https://bugs.openjdk.java.net/browse/JDK-8024423</a><br>
          Webrev: <a moz-do-not-send="true"
            href="http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.00/">http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/</a><br>
          <br>
          In this review I still have an outstanding unanswered question
          about the need to synchronize the access to the:<br>
          <pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span class="new" style="color: blue; font-weight: normal;">ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);
<font color="#000000">and</font>
</span><span class="new" style="color: blue; font-weight: normal;">ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);</span>

calls in 
<a moz-do-not-send="true" href="http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.00/src/share/vm/prims/jvmtiGetLoadedClasses.cpp.udiff.html">http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/src/share/vm/prims/jvmtiGetLoadedClasses.cpp.udiff.html</a>

Please give me advise on this.

There will be a review soon regarding re-enabling the tests that failed because of the above bug, see:
<a moz-do-not-send="true" href="https://bugs.openjdk.java.net/browse/JDK-8025576">https://bugs.openjdk.java.net/browse/JDK-8025576</a>

Cheers
/Fredrik
</pre>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>