[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-dev
Subject:    Re: RFR (M) 8210155: Lock ClassLoaderDataGraph
From:       coleen.phillimore () oracle ! com
Date:       2018-08-30 20:45:31
Message-ID: f776a87e-5a7e-e174-5204-ce78a4579171 () oracle ! com
[Download RAW message or body]



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

[prev in list] [next in list] [prev in thread] [next in thread]