RFR(XS) for PeriodicTask_lock cleanup (8072439)

Daniel D. Daugherty daniel.daugherty at oracle.com
Fri Feb 27 21:54:25 UTC 2015 

Coleen and David,

My final attempt to get more acceptable wording for this comment:

Here's the current wording:

+    // The WatcherThread is not a JavaThread so we do not honor the
+    // safepoint protocol for the PeriodicTask_lock.
      MutexLockerEx ml(PeriodicTask_lock, Mutex::_no_safepoint_check_flag);

How about:

+    // The WatcherThread does not participate in the safepoint protocol
+    // for the PeriodicTask_lock because it is not a JavaThread.
      MutexLockerEx ml(PeriodicTask_lock, Mutex::_no_safepoint_check_flag);

This change would be made in task.cpp: PeriodicTask::real_time_tick()
and thread.cpp: WatcherThread::sleep().

Is this acceptable?

Dan


On 2/27/15 1:36 PM, Coleen Phillimore wrote:
>
> On 2/27/15, 10:46 AM, Daniel D. Daugherty wrote:
>> Coleen,
>>
>> Thanks for the review. Replies embedded below.
>>
>> David,
>>
>> Thanks for jumping in while I was out of the office yesterday.
>>
>>
>> On 2/26/15 12:13 AM, David Holmes wrote:
>>> On 26/02/2015 2:46 PM, Coleen Phillimore wrote:
>>>>
>>>> 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.
>>>
>>> Yes, internally there is one path for JavaThreads and a different 
>>> path for non-JavaThreads. JavaThreads perform thread-state 
>>> transitions which form part of the safepoint protocol.
>>
>> So it looks like "honor the safepoint protocol" is bad choice of
>> words. More below.
>>
>> Earlier in this thread, Marcus talked about switching from
>> MutexLockerEx -> MutexLocker and the problem that he ran into
>> with the wait() function. He's planning to explore this idea
>> further using a different bug ID in order to clean up this
>> wart.
>>
>>
>
> +  // The WatcherThread is not a JavaThread so we do not honor the
> +  // safepoint protocol for the PeriodicTask_lock.
>     MutexLockerEx ml(PeriodicTask_lock, Mutex::_no_safepoint_check_flag);
> Your explanation makes sense.   To be honest, I'm not sure how to 
> reword it so it makes more sense, other than to add that the safepoint 
> check isn't made anyway for !JavaThreads.
>
> Coleen
>>>
>>>> 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 don't read it that way but would it be better if it said "do not 
>>> _need to_ honor" instead?
>>
>> Perhaps "does not participate in the safepoint protocol" is
>> better and more clear?
>>
>>
>>>
>>> David
>>> -----
>>>
>>>> I'll stop reading comments again.
>>
>> Please don't. I'm trying to make the comments more clear in an
>> area that is a bit messy. If the comments don't make sense to
>> you, then I need to fix that.
>>
>>
>>>>
>>>> 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.
>>
>> WatcherThread::stop() does a safepoint check because it is
>> called by a JavaThread instead of the WatcherThread. I added
>> a new comment that was supposed to make this clear. Is the
>> comment in thread.cpp not clear?
>>
>>
>>>>>> It's unclear
>>>>>> whether enroll and disenroll can be called by the WatcherThread 
>>>>>> also.
>>>>>> If so, should they have no_safepoint_check.
>>
>> That's true. It is not clear that the WatcherThread does not call
>> enroll() or disenroll() and I didn't add an assert for this one.
>> As David pointed out earlier in this review thread, the API for
>> the PeriodicTask class needs to be cleaned up a bit. I concur,
>> but we agreed that should be done via another bug ID that is
>> focused on just API cleanup.
>>
>>
>>>>>
>>>>> 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 for taking a look at it anyway.
>>
>> Dan
>>
>>
>>>>>>
>>>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20150227/4f394f56/attachment.html>

More information about the serviceability-dev mailing list