RFR(XS) for PeriodicTask_lock cleanup (8072439)

[Coleen Phillimore]

On 2/25/15, 10:59 PM, David Holmes wrote:
> On 26/02/2015 1:44 PM, Coleen Phillimore wrote:
>>
>> Hi, I think you have better reviewer for this now but I did have a
>> couple of questions.
>>
>> in
>> http://cr.openjdk.java.net/~dcubed/8072439-webrev/2-for_jdk9_hs_rt/src/share/vm/runtime/task.cpp.udiff.html 
>>
>>
>> This comment is really confusing because I think you do in fact honor
>> the safepoint protocol sometimes for the WatcherThread, true?
>
> Depends what you mean by "honor the safepoint protocol". The 
> WatcherThread will sometimes acquire locks that potentially check the 
> safepoint state, but as it is not a JavaThread such paths are never 
> actually taken. Hence we can call the code that will never consider 
> taking them if the only caller of that code (as in this case) is a 
> non-JavaThread.

So you could theoretically have the watcher thread use the regular 
MutexLocker, which checks for safepoints, and somewhere behind the 
scenes, the safepoint check is elided.

So the comment really doesn't explain what's going on, and leads one to 
believe that you need to be careful _not_ to take a safepoint check with 
the Watcher thread.

I'll stop reading comments again.

Thanks for the explanation though, the latter part I knew about.

Coleen

>
>>    65     // The WatcherThread is not a JavaThread so we do not honor 
>> the
>>    66     // safepoint protocol for the PeriodicTask_lock.
>>    67     MutexLockerEx ml(PeriodicTask_lock, 
>> Mutex::_no_safepoint_check_flag);
>>
>>
>> Because WatcherThread::stop() does safepoint check.   It's unclear
>> whether enroll and disenroll can be called by the WatcherThread also.
>> If so, should they have no_safepoint_check.
>
> You can only use MutexLockerEx with _no_safepoint_check_flag if you 
> never want to check for safepoints. If the only caller is a 
> non-JavaThread then that is trivially true. If the caller can be a 
> JavaThread then safepoint checking can only be elided under very 
> specific circumstances (ie leaf methods with other constraints). If 
> the lock is to be acquired by JavaThreads, as is the case with 
> enrol/disenrol then we should check for safepoints (as the conditions 
> for eliding safepoint checks for JavaThreads are not met by those 
> methods).
>
>> The whole safepoint checking inconsistency still makes me nervous, but
>> the comment seems misleading.
>
> Hope it is clearer now.
>
> David
>
>>
>> I honestly don't know enough about the rest to review it.
>>
>> thanks,
>> Coleen
>>
>> On 2/25/15, 12:00 PM, Daniel D. Daugherty wrote:
>>> This should be the last webrev:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8072439-webrev/2-for_jdk9_hs_rt/
>>>
>>> Coleen, since you were one of my reviewers on JDK-8047720, I'd like
>>> to hear from you in this hopefully final round...
>>>
>>> Dan
>>>
>>>
>>>
>>> On 2/18/15 10:00 AM, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> Here is an updated webrev after addressing David H's comments:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8072439-webrev/1-for_jdk9_hs_rt/
>>>>
>>>> Also, here is the bug's URL:
>>>>
>>>> JDK-8072439 fix for 8047720 may need more work
>>>> https://bugs.openjdk.java.net/browse/JDK-8072439
>>>>
>>>> Update for testing: I'm taking the new Remote Build and Test (RBT)
>>>> system for a ride during its beta period so I won't be doing direct
>>>> Aurora Adhoc jobs...
>>>>
>>>> Dan
>>>>
>>>>
>>>> On 2/17/15 2:44 PM, Daniel D. Daugherty wrote:
>>>>> Greetings,
>>>>>
>>>>> My fix for the following bug:
>>>>>
>>>>>     JDK-8047720 Xprof hangs on Solaris
>>>>>
>>>>> that was pushed to JDK9 last June needs to be cleaned up.
>>>>>
>>>>> Thanks to Alex Garthwaite (agarthwaite at twitter.com) and Carsten
>>>>> Varming (varming at gmail.com) for reporting the mess that I made
>>>>> in WatcherThread::stop() and for suggesting fixes.
>>>>>
>>>>> This code review is for a general cleanup pass on PeriodicTask_lock
>>>>> and some of the surrounding code. This is a targeted review in that
>>>>> I would like to hear from three groups of people:
>>>>>
>>>>> 1) The author and reviewers for:
>>>>>
>>>>>    JDK-7127792 Add the ability to change an existing PeriodicTask's
>>>>>                execution interval
>>>>>
>>>>>    Rickard, David H, and Markus G.
>>>>>
>>>>> 2) The reviewers for:
>>>>>
>>>>>    JDK-8047720 Xprof hangs on Solaris
>>>>>
>>>>>    Markus G and Coleen
>>>>>
>>>>> 3) Alex and Carsten
>>>>>
>>>>>
>>>>> Here's the webrev URL:
>>>>>
>>>>> http://cr.openjdk.java.net/~dcubed/8072439-webrev/0-for_jdk9_hs_rt/
>>>>>
>>>>> I've attached the original RFR for JDK-8047720 that explains
>>>>> the original deadlock that was being fixed. Similar testing
>>>>> will be done with this fix.
>>>>>
>>>>> Dan
>>>>
>>>>
>>>>
>>>
>>