RFR 8209821: Make JVMTI GetClassLoaderClasses not walk CLDG

[Lois Foltan]

On 8/25/2018 11:38 AM, coleen.phillimore at oracle.com wrote:

>
>
> On 8/24/18 4:38 PM, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 8/24/18 3:31 PM, Lois Foltan wrote:
>>> On 8/24/2018 2:59 PM, coleen.phillimore at oracle.com wrote:
>>>
>>>>
>>>>
>>>> On 8/24/18 11:44 AM, Lois Foltan wrote:
>>>>> On 8/23/2018 8:37 AM, coleen.phillimore at 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.
>
> This code in metaspace_pointers_do and serialize has to fill in the 
> values for the 0-3 entries (ie NULL), and up to T_VOID, which are also 
> NULL.
>
> Rereading your mail again, I did implement your suggestion: for (int i 
> = T_BOOLEAN; i < T_LONG+1; i++).  And it passes all the tests.

Thanks!  Looks good.  I'm glad you opened an RFE to address the other 
uses of _typeArrayKlassObjs in universe.cpp.
Lois

>
> Thanks,
> Coleen
>>
>> 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
>>>>
>>>
>>
>