RFR(XS) for PeriodicTask_lock cleanup (8072439)

Fri Feb 27 15:46:37 UTC 2015

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.

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