RFR (M) 8210155: Lock ClassLoaderDataGraph

[Harold David Seigel]

That sounds good!

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