RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes
Fredrik Arvidsson
fredrik.arvidsson at oracle.com
Tue Oct 15 05:08:58 PDT 2013
I think this looks great.
/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/20131015/47f581dc/attachment-0001.html
More information about the serviceability-dev
mailing list