[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR 8209821: Make JVMTI GetClassLoaderClasses not walk CLDG
From:       coleen.phillimore () oracle ! com
Date:       2018-08-24 20:38:14
Message-ID: 9b59cad6-211f-ce9b-fef8-f65304a49250 () oracle ! com
[Download RAW message or body]



On 8/24/18 3:31 PM, Lois Foltan wrote:
> On 8/24/2018 2:59 PM, coleen.phillimore@oracle.com wrote:
>
>>
>>
>> On 8/24/18 11:44 AM, Lois Foltan wrote:
>>> On 8/23/2018 8:37 AM, coleen.phillimore@oracle.com wrote:
>>>
>>>> Summary: And also added function with KlassClosure to remove the 
>>>> hacks.
>>>>
>>>> There are about 10 vmTestbase/nsk/jvmti tests that test various 
>>>> parts of this change.   Also ran mach5 tier1-7.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8209821.01/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8209821
>>>>
>>>> Thanks,
>>>> Coleen
>>>
>>> Hi Coleen,
>>>
>>> I think this is a good clean up.   Couple of comments.
>>>
>>> - memory/universe.cpp
>>> You could make basic_type_classes_do() be a for loop
>>>       for (int i = 0; i < T_VOID+1; i++) {
>>>            closure->do_klass(typeArrayKlassObjs[i]());
>>>    }
>>
>> Interesting observation.   This is equivalent except T_OBJECT and 
>> T_ARRAY elements aren't initiatialized.   I believe that the do_klass 
>> in the closure I am passing will choke on NULL.   I've never 
>> understood why these statics needed to be duplicated like this, and 
>> have tried to clean this up before.   Maybe an RFE to do so would be 
>> better.
> Hi Coleen,
>
> Then just have the for loop be instead   for (int i = T_BOOLEAN; i < 
> T_LONG+1; i++).   I'm okay with what you decide.   However, to me the 
> code in Universe::metaspace_pointers_do() at line 230-232 looks wrong:
>
> for (int i = 0; i < T_VOID+1; i++) {
>        it->push(&_typeArrayKlassObjs[i]);
>    }
>
> The VM does not appear to store anything in _typeArrayKlassObjs for 
> elements 0-3, so hopefully those elements are NULL.   It should at 
> least start that for loop off at T_BOOLEAN.

Hm, you're right T_BOOLEAN starts at 4.   The closure will still crash on 
zero for _typeArrayKlassObj[T_ARRAY] and [T_OBJECT]. Actually 
_typeArrayKlassObj only goes to T_LONG, so I could change the loop to be 
from T_BOOLEAN to T_LONG inclusively (and rerunning tests).

http://cr.openjdk.java.net/~coleenp/8209821.02/webrev/src/hotspot/share/memory/universe.cpp.udiff.html

I opened this RFE to clean these up some more.

https://bugs.openjdk.java.net/browse/JDK-8209958

Thanks,
Coleen
>
> Thanks,
> Lois
>
>
>>>
>>> - prims/jvmtiGetLoadedClasses.cpp
>>> In JvmtiGetLoadedClasses::getClassLoaderClasses() you could pull the 
>>> call to basic_type_classes_do() from both sections of the if 
>>> statement to line #139
>>
>> Thanks, I'll fix it.
>>
>> Thanks for the code review.
>> Coleen
>>>
>>> Thanks,
>>> Lois
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]