RFR 8209821: Make JVMTI GetClassLoaderClasses not walk CLDG

[coleen.phillimore at oracle.com]

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