RFR(S): 8061256: com/sun/management/DiagnosticCommandMBean/DcmdMBeanPermissionsTest.java timed out

[Nils Eliasson]

Hi Albert,

I had forgotten to remove the lock from the constructor as you pointed out.

Updated webrev:
http://cr.openjdk.java.net/~neliasso/8061256/webrev.03/

Thanks,
Nils

On 2014-11-17 13:03, Albert Noll wrote:
> Hi Nils,
>
> On 11/17/2014 12:52 PM, Nils Eliasson wrote:
>> Hi Vladimir,
>>
>> I just think the code is easier to read and browse this way. If we 
>> want separate queue locks in the future it is easy to add back.
>>
>> If you and Albert don't want that part, I'll remove it.
>>
> I have no strong opinion about this - I just think the code is fine as 
> it is. If you want to go for it, that's ok for me.
>
> Best,
> Albert
>
>> Regards,
>> //Nils
>>
>>
>>
>> On 2014-11-14 19:01, Vladimir Kozlov wrote:
>>> Hi Nils,
>>>
>>> Looks like you answered Albert's questions.
>>> I am not happy with refactoring lock() accessor for the same reason 
>>> as Albert said.
>>> But I understand why you did it and fine with that.
>>> We can investigate separate locks for queues but I don't think it is 
>>> a bottleneck currently. Also some parts of code relay on one lock.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 11/13/14 6:11 AM, Nils Eliasson wrote:
>>>> Hi,
>>>>
>>>> Please review this small change.
>>>>
>>>> 1) Fixing a deadlock in diagnostic command dcmd 
>>>> print_compile_queues - between the CompileQueue_lock and safepointing.
>>>> The CompileQueue_lock requires that we are not at a safepoint when 
>>>> taking it. Otherwise a java-thread that already holds
>>>> the lock will block when trying to get another lock. In this 
>>>> specific case the compiler threads are Java threads, they
>>>> take the CompileQueue_lock frequently and some time takes the 
>>>> CompileTaskAlloc_lock after that.
>>>>
>>>> Fixing this by making the diagnostic command not request a safepoint,
>>>>
>>>> 2) Refactoring away the lock() accessor in CompileQueue and stating 
>>>> the lock explicitly. This removes an element of
>>>> surprise and makes it easier understanding the code.
>>>>
>>>> webrev: http://cr.openjdk.java.net/~neliasso/8061256/webrev.02/
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8061256
>>>>
>>>> Thanks,
>>>> Nils Eliasson
>>>>
>>
>