RFR 8209821: Make JVMTI GetClassLoaderClasses not walk CLDG

[coleen.phillimore at oracle.com]

On 8/27/18 10:15 AM, Lois Foltan wrote:
> 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, Lois!
Coleen

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