RFR(XS) for PeriodicTask_lock cleanup (8072439)

Daniel D. Daugherty daniel.daugherty at oracle.com
Mon Mar 2 15:49:15 UTC 2015


On 3/2/15 1:27 AM, David Holmes wrote:
> 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!

Dan


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