RFR(XS) for PeriodicTask_lock cleanup (8072439)

Tue Feb 24 21:26:10 UTC 2015

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.

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

> 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 upfor this work.

> 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()...

> 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 aboutthis instead:

   // volatile due to at least one lock-free read
   volatile static bool _should_terminate;

> 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; hotspot-runtime-dev at openjdk.java.net; 
> serviceability-dev at 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 
> <daniel.daugherty at oracle.com <mailto:daniel.daugherty at 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 (agarthwaite at twitter.com 
> <mailto:agarthwaite at twitter.com>) and Carsten
> Varming (varming at gmail.com <mailto: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/ 
> <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
>