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
