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.
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'll stop reading comments again.
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. It's unclear
whether enroll and disenroll can be called by the WatcherThread also.
If so, should they have no_safepoint_check.
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,
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
