More replies embedded below...
On 2/24/15 10:02 AM, Markus Gronlund wrote:
Actually thinking about this a bit more:
I think we could make all uses of PeriodicTask_lock to be acquired
with MutexLocker (not Ex), and avoid passing the
Mutex::_no_safepoint_check flag (and lengthy comments):
JavaThreads will (check for and) block for safepoints in
WatcherThread::enroll/disenroll if the PeriodicTask_lock is being held
by someone else. Same thing in before_exit().
Since the WatcherThread is not a JavaThread and will never check for a
safepoint if there is a contended lock, it will call IWait() (to park)
directly.
This would also make it possible to change the PeriodicTask_lock from
being asserted as a “_safepoint_check_sometimes” to a
“_safepoint_check_always”.
Coleen (and Max) would like that... :-)
In order to do this however, we would need to rework Monitor::Wait():
The only place (currently) where there is a requirement to pass
“Mutex::_no_safepoint_check” is when the WatcherThread calls Wait() –
but this is because we have this in there:
// !no_safepoint_check logically implies java_thread
guarantee(no_safepoint_check || Self->is_Java_thread(), "invariant");
This does not make sense – a WatcherThread should not need to
explicitly say “please go outside the safepoint protocol” - it is not
a JavaThread so to it, there is no such thing as a safepoint.
That's why MutexLockerEx exists... but I see your point.
We're evolving this area with the _safepoint_check_* stuff
so why not make MutexLocker smarter...
In Monitor::lock() we branch to a safepoint check based on the
Self->isJavaThread(), but in Monitor::wait() we also allow for
JavaThreads to circumvent the protocol if they pass in the correct flag.
This is definitely a little inconsistent.
Maybe we can change Monitor::Wait() a wee bit (I know this is
sensitive code), and still allow for arbitrary passings of
“no_safepoint_checks” for JavaThreads, but if there is nothing passed,
we take the safepoint route if there is a JavaThread, and not if there
is anything else (similar to Monitor::lock()). Callers which are not
JavaThreads should not need to pass these options. Combining this with
the lock assertion states, such as, “_safepoint_check_always” will
disallow anyone (any JavaThread) to circumvent the safepoint protocol
for the PeriodicTask_lock.
Yes I agree this can likely be cleaned up a bit...
I will try some experiments, so Dan please go ahead with what you
already have.
So I'll leave the further cleanup to your experiment
and a new bug. I'll move forward with the webrev plus
the tweaks I identified in my reply to your first e-mail.
Thanks againfor the reviews!
Dan
Cheers
Markus
*From:*Markus Gronlund
*Sent:* den 24 februari 2015 16:13
*To:* Daniel Daugherty
*Cc:* Alexander Garthwaite; Rickard Bäckman; David Holmes; Coleen
Phillimore; [email protected];
[email protected]; Carsten Varming
*Subject:* RE: RFR(XS) for PeriodicTask_lock cleanup (8072439)
Hi Dan,
I have taken a look with your suggested patch – I think your
suggestion looks very good.
I guess the original hang happened because the PeriodicTask_lock was
attempted to be acquired by a JavaThread, but the PeriodicTask_lock
was still held by someone else. Since the PeriodicTask_lock was taken
with “Mutex::_no_safepoint_checks” it meant the JavaThread bypassed
the callback for a potentially pending safepoint and instead called
parked upon the PeriodicTask_lock straight away...
I think this lock should definitely be taken the way you have done in
the patch.
I also think the placement of OrderAccess::fence() might have been due
to some of the constructs being racy, take this for instance:
void WatcherThread::start() {
assert(PeriodicTask_lock->owned_by_self(), "PeriodicTask_lock
required");
if (watcher_thread() == NULL && _startable) { _startable is visible
since its the same thread
_should_terminate = false; <<----------------------------- this is
set but will not be visible to the WatcherThread being launched (it’s
a 0 in the static initializer however, so it is still “safe”)
// Create the single instance of WatcherThread
new WatcherThread();
// above the constructor for WatcherThread will start the thread, and
the WatcherThread::run() might check _should_terminate before the
launching thread releases the PeriodicTask_lock. Not that it will be
an issue here, since _should_terminate is set to 0 in its static
initializer. But thanks Dan for moving this _should_terminate lower in
the loop, at least the WatcherThread will need now need a call to
sleep() before reaching it (and sleep needs the PeriodicTask_lock)
But for the construct in WatcherThread::stop(), there is no need (any
more?) for the OrderAccess::fence(), I think it can be safely removed.
Can you also remove the comment in thread.hpp : 704 that says:
volatile static bool _should_terminate; // updated without holding lock
As this is not the case any longer.
Otherwise it looks good!
Thanks for fixing this
Cheers
Markus
*From:*Daniel D. Daugherty
*Sent:* den 17 februari 2015 23:42
*To:* Carsten Varming
*Cc:* Alexander Garthwaite; Rickard Bäckman; David Holmes; Markus
Grönlund; Coleen Phillimore; [email protected]
<mailto:[email protected]>;
[email protected]
<mailto:[email protected]>
*Subject:* Re: RFR(XS) for PeriodicTask_lock cleanup (8072439)
On 2/17/15 3:22 PM, Carsten Varming wrote:
Dear Daniel,
Looks good to me.
Thanks for the fast review.
The line: "OrderAccess::fence(); // ensure WatcherThread sees update
in main loop" seems unnecessary as the lock acts as a memory barrier.
Yes, I keep looking at that line from the original work on
JDK-7127792 and wonder why it's there... I'll chase that down
with the original folks...
Dan
Carsten
On Tue, Feb 17, 2015 at 4:44 PM, Daniel D. Daugherty
<[email protected] <mailto:[email protected]>> 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 ([email protected]
<mailto:[email protected]>) and Carsten
Varming ([email protected] <mailto:[email protected]>) 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/
<http://cr.openjdk.java.net/%7Edcubed/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
