Thanks again Dan,
Added some comments, pls see below.
/Markus
From: Daniel D. Daugherty
Sent: den 24 februari 2015 22:26
To: Markus Gronlund
Cc: Alexander Garthwaite; Rickard Bäckman; David Holmes; Coleen Phillimore;
hotspot-runtime-...@openjdk.java.net; serviceability-dev@openjdk.java.net;
Carsten Varming
Subject: Re: RFR(XS) for PeriodicTask_lock cleanup (8072439)
Markus,
Thanks for the review. Replies embedded below.
On 2/24/15 8:13 AM, Markus Gronlund wrote:
Hi Dan,
I have taken a look with your suggested patch – I think your suggestion looks
very good.
Thanks.
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...
Not sure which "original hang" you are referencing. There's
lots of gory details about the hang I fixed in JDK-8047720.
[MG] I meant the JDK-8047720 hang – wouldn’t you say all hangs are a bit
“original” ? :)
I think this lock should definitely be taken the way you have done in the patch.
Glad we're on the same page.
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.
The _should_terminate field is volatile and this is a
"single writer" situation relative to the WatcherThread
being a "single reader". Even if _should_terminate was
initialized to 99 in the static initializer, this line:
1322 _should_terminate = false;
should be visible to the newly created WatcherThread on
these lines:
1324 new WatcherThread();
:
(old) 1255 while (!_should_terminate) {
[MG] I’m sorry, i overlooked the volatility of the variable – thanks – and I
liked your updated comment regarding it (below).
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)
This was one of Carsten's suggested changes which I
picked up for this work.
Good work Carsten!
But for the construct in WatcherThread::stop(), there is no need (any more?)
for the OrderAccess::fence(), I think it can be safely removed.
Do you (or Rickard) know why it was there in the first
place? Even in the original code for JDK-7127792 we should
not have needed the fence()...
[MG] I honestly don’t remember – as you point out it does not make any real
sense in this context, so I can mostly assume a non-intentional
change/something left in from previous experiments / lax reviewing on my part –
maybe Rickard remembers if there was a real need for this..
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.
How about this instead:
// volatile due to at least one lock-free read
volatile static bool _should_terminate;
[MG] yep. Good.
Otherwise it looks good!
Thanks!
Thanks for fixing this
I messed it up with my fix for JDK-8047720 so should
clean that up... :-)
Dan
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; HYPERLINK
"mailto:hotspot-runtime-...@openjdk.java.net"hotspot-runtime-...@openjdk.java.net;
HYPERLINK
"mailto:serviceability-dev@openjdk.java.net"serviceability-dev@openjdk.java.net
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 <HYPERLINK
"mailto:daniel.daughe...@oracle.com"; \ndaniel.daughe...@oracle.com> 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 (HYPERLINK "mailto:agarthwa...@twitter.com";
\nagarthwa...@twitter.com) and Carsten
Varming (HYPERLINK "mailto:varm...@gmail.com"; \nvarm...@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:
HYPERLINK
"http://cr.openjdk.java.net/%7Edcubed/8072439-webrev/0-for_jdk9_hs_rt/";
\nhttp://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
