RFR(XS) for PeriodicTask_lock cleanup (8072439)

David Holmes david.holmes at oracle.com
Mon Mar 2 08:27:03 UTC 2015


On 28/02/2015 7:54 AM, Daniel D. Daugherty wrote:
> 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?

Fine by me.

Thanks,
David

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


More information about the hotspot-runtime-dev mailing list