I think this looks great.
/Fredrik
On 2013-10-15 09:42, serguei.spit...@oracle.com wrote:
There are two questions on the list that we still need to resolve in
this fix:
(1) What is the best way to protect the work with CLDG from
concurrent additions of CLD's to it
(2) The *GetClassLoaderClasses* needs a fix as well to be consistent
with the GetLoadedClasses
I had some private conversations with Fredrik and John Rose and
after some analysis came up with the suggestion:
(1) Continue using the *SystemDictionary_lock* to protect
consistency of the loaded classes data.
The issue is that adding CLD's to the SLDG is not currently
protected by the *SystemDictionary_lock*.
I'm suggesting to add it to the
*SystemDictionary::parse_stream* (please, see the webrev below).
(2) There was some doubt that a similar fix for the
*GetClassLoaderClasses* is needed because
the CL+SD (taken from the host class) does not reference the
associated anonymous classes.
The question is: Can we consider the host's class CL as the
initiating CL?
My reading is that the answer to this question is: "Yes, we can".
Even though the CL itself does not have references to its
anonymous classes,
there are references from the anonymous classes's CLD's to its CL.
These references can be used in the *increment_with_loader* and
*add_with_loader*
the same way as it was before.
This is a webrev that includes the Fredrik's fix as a base plus the
implemented suggestion:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2013/hotspot/fredrik/8024423-JVMTI-ANO/
Some help from the HotSpot team is needed to estimate if this approach
is Ok and does not have rat holes in it.
Opinions are very welcome.
Thanks,
Serguei
On 10/2/13 2:28 AM, Fredrik Arvidsson wrote:
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.spit...@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
