RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Oct 23 08:18:14 UTC 2013
Fredrik,
The fix is good, good job!
It is still a good idea to run the nsk.jvmti and nsk.jdi tests to be safe.
Thanks,
Serguei
On 10/23/13 1:04 AM, Fredrik Arvidsson wrote:
> 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: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20131023/408184bb/attachment.htm>
More information about the hotspot-gc-dev
mailing list