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 (agarthwa...@twitter.com) and Carsten
Varming (varm...@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
