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

[Albert Noll]

Hi Nils,

On 11/13/2014 04:39 PM, Nils Eliasson wrote:
> 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.
It is a property of the compile queue to have a lock. As a result, it 
makes sense for the compile queue to keep a reference to that lock. I 
would argue that this keeps the code more maintainable compared to 
referencing a global variable directly. However, I don't have a strong 
opinion here.

In any case, if you decide to go for this refactoring, you should also 
adapt the constructor of CompileQueue(const char* name, Monitor* lock).

Best,
Albert

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