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

[Fredrik Arvidsson]

Hi and thanks for all your comments.

I have simplified the code in *instanceKlass.cpp* like suggested and 
here is a new webrev:
http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/ 
<http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.01/>

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.

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.

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 :)

Cheers
/Fredrik

On 2013-10-01 09:34, serguei.spitsyn at oracle.com wrote:
> 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/20131002/49255ad9/attachment-0001.html