RFR (M) 8210155: Lock ClassLoaderDataGraph

[coleen.phillimore at oracle.com]

On 8/30/18 4:25 PM, Harold David Seigel wrote:
> That sounds good!
Thanks, Harold.  I'll give it a last mach5 spin but I only changed these 
three small things.

Coleen
>
> Thanks, Harold
>
>
> On 8/30/2018 3:59 PM, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 8/30/18 1:13 PM, Harold David Seigel wrote:
>>> Hi Coleen,
>>>
>>> This looks good.  Just a couple of small questions.
>>>
>>> In classLoaderData.cpp, can you get rid of line 1096 and change 1097 
>>> to cld->set_next(_head);  ?
>>>
>>>    1096   ClassLoaderData* next = _head;
>>>    1097   cld->set_next(next);
>>>    1098   _head = cld;
>>
>> Yes, fixed that.
>>>
>>> In some places, it's unclear when a ClassLoaderDatagraph_lock is 
>>> needed.  For example, why does CLDG::methods_do() need the lock but 
>>> CLDG::classes_do() doesn't ?  Can you add a few comments?
>>
>> I wanted to take out the lock in the CLDG::blah_do functions directly 
>> but they are called in various contexts, including during safepoint, 
>> so the caller needs to take the lock if it is outside a safepoint.
>>
>> CLDG::methods_do was inconsistent though.  I moved the lock into its 
>> one caller.   I added the comment:
>>
>> // These functions assume that the caller has locked the 
>> ClassLoaderDataGraph_lock
>> // if they are not calling the function from a safepoint.
>>
>>>
>>> In CMSCollector::preclean_cld(), do you need to hold the lock for 
>>> the whole function or just until cld_do() is done?  Same question 
>>> for compute() in klassVtable.cpp.
>>
>> For the CMS function, the lock has to be held outside the 
>> CMSTokenSyncWithLocks because of rankings, so I can't change that. 
>> klassVtable.cpp compute does little after taking the lock, so it 
>> doesn't seem worth putting it into a scope by itself.
>>
>> Thank for reviewing!
>> Coleen
>>>
>>> Thanks! Harold
>>>
>>> On 8/29/2018 12:07 PM, coleen.phillimore at oracle.com wrote:
>>>> Summary: In preparation for concurrent class unloading.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8210155.01/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8210155
>>>>
>>>> Tested with mach5 hs-tier1-7, which tests JFR and JVMTI changes. 
>>>> Also tested with performance dev-submit testing with no regression.
>>>>
>>>> Thanks,
>>>> Coleen
>>>
>>
>