RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes

[serguei.spitsyn at oracle.com]

Hi Frederik,


Thank you for jumping on this issue!


*src/share/vm/oops/instanceKlass.cpp*

2333   int src_index = 0;
2334   while (src_index < src_length) {
2335     dest[dest_index++] = src[src_index++];
2336   }
2337
2338   // If we have a hash, append it
2339   if(hash_len > 0) {
2340     int hash_index = 0;
2341     while (hash_index < hash_len) {
2342       dest[dest_index++] = hash_buf[hash_index++];
2343     }
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 conditionif(hash_len > 0)  is covered by the loop itself.

Style-related comments:
  -wrong indent at the lines: 2316-2319
  - need a space after the 'if' at the lines: 2315, 2339


*src/share/vm/prims/jvmtiGetLoadedClasses.cpp

*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:
     
> ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);
> and
> ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);

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
*
*On 9/30/13 4:45 AM, Fredrik Arvidsson wrote:

> Hi
>
> Please help me to review the changes below:
>
> Jira case: https://bugs.openjdk.java.net/browse/JDK-8024423
> Webrev: http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/ 
> <http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.00/>
>
> In this review I still have an outstanding unanswered question about 
> the need to synchronize the access to the:
> ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);
> and
> ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);
>
> calls in
> http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/src/share/vm/prims/jvmtiGetLoadedClasses.cpp.udiff.html  <http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.00/src/share/vm/prims/jvmtiGetLoadedClasses.cpp.udiff.html>
>
> 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:
> https://bugs.openjdk.java.net/browse/JDK-8025576
>
> Cheers
> /Fredrik

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20131001/4e739635/attachment.html