RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes
Fredrik Arvidsson
fredrik.arvidsson at oracle.com
Wed Oct 23 01:04:45 PDT 2013
Hi
The latest and hopefully last review version of this change can be found
here:
http://cr.openjdk.java.net/~farvidsson/8024423/webrev.03/
<http://cr.openjdk.java.net/%7Efarvidsson/8024423/webrev.03/>
Cheers
/F
On 2013-10-22 17:11, Stefan Karlsson wrote:
> On 10/17/13 4:02 PM, Fredrik Arvidsson wrote:
>> Hi
>>
>> I have added a new revision of my changes here:
>> http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.02/
>> <http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.02/>
>
> I haven't looked at the patch i great detail, but I found this code:
>
> + void LoadedClassesClosure::do_klass(Klass* k) {
> + // Collect all jclasses
> + for (Klass* l = k; l != NULL; l = l->array_klass_or_null()) {
> + oop mirror = l->java_mirror();
> + _classStack.push((jclass) _env->jni_reference(mirror));
> + }
> + }
>
> I think you are adding the array klasses multiple times.
> ClassLoaderData::loaded_classes_do() is visiting all array klasses, so
> you shouldn't be traversing the l->array_klass_or_null() chain.
>
> StefanK
>
>> The idea is to use a lock called metaspace_lock available inside the
>> ClassLoaderData objects that the ClassLoaderDataGraph holds, this
>> prevents classes to be added/removed/updated during the gathering
>> phase. When iterating using the new LoadedClassesClosure
>> implementation only array classes and instance classes that are
>> marked as loaded will be visited. The LoadedClassesClosure
>> implementation uses a Stack<jclass> to store classes during the
>> gathering phase. This changes the count-allocate-add-extract approach
>> that was used before into a add-extract way of doing it instead.
>>
>> I am still not sure how to do with the GetClassLoaderClasses to make
>> it consistent. I would eventually like to get rid of the
>> JvmtiGetLoadedClassesClosure and the use of the SystemDictionary if
>> possible. But right now I just cant see how to get hold of the
>> initiating loader for a class without using the SystemDictionary.
>>
>> /Fredrik
>>
>> On 2013-10-15 09:42, serguei.spitsyn at 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.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/20131023/426d9db5/attachment-0001.html
More information about the serviceability-dev
mailing list