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

[Nils Eliasson]

Hi Albert,

Yes exactly. When there is only one lock, there is no good reason for 
adding it to both queues. (That only make sense with separate locks.) It 
is much easier to find all usages of MethodCompileQueue_lock with the patch.

Regards,
//Nils

On 2014-11-13 15:58, Albert Noll wrote:
> Hi Nils,
>
> CompileQueue::lock() always returns MethodCompileQueue_lock ( see 
> init_compiler_sweeper_threads() ). It seems that your changes don't 
> change existing behavior, or am I missing something? Note that we have 
> 2 compilation queues, but only 1 lock.
>
> On 11/13/2014 03:11 PM, 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.
> What element of surprise are you talking about? Personally, I prefer 
> the current design, i.e., getting the lock by calling a function 
> instead of referencing a global variable directly. If we ever decide 
> to go for a 2 compile queue locks (I think that makes sense since we 
> have two compilation queues), we would have to introduce the functions 
> again.
>
> Best,
> Albert
>
>
>> webrev: http://cr.openjdk.java.net/~neliasso/8061256/webrev.02/
>> bug: https://bugs.openjdk.java.net/browse/JDK-8061256
>>
>> Thanks,
>> Nils Eliasson
>>
>