RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Oct 18 03:39:18 PDT 2013
Hi Fredrik,
I like this approach in general but am still looking into some details.
There is also one concern from Coleen about the anonymous classes:
they appear in the CLDG/CLD partially initialized.
For details, look at the function SystemDictionary::parse_stream()
that is called from the Unsafe_DefineAnonymousClass_impl().
Thanks,
Serguei
On 10/17/13 7:02 AM, 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/>
>
> 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/20131018/56a218b0/attachment.html
More information about the serviceability-dev
mailing list