RFR(XS) for PeriodicTask_lock cleanup (8072439)

[David Holmes]

Hi Dan,

A few comments, mainly on the comments :)

On 18/02/2015 8:42 AM, Daniel D. Daugherty wrote:
> 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...

Only needed when using lock-free access, so can be removed now.

java.cpp:

Typo: likelyhood -> likelihood

---

task.cpp

  81 int PeriodicTask::time_to_wait() {
  82   assert(Thread::current()->is_Watcher_thread(), "must be 
WatcherThread");

This is currently true but not sure it has to be true. If we are 
assuming/constraining a subset of the task API to only be callable by 
the WatcherThread then perhaps we should document that in task.hpp ? And 
as the WatcherThread is a friend, none of those methods need to be 
public - ie execute_if_pending and time_to_wait (realtime_tick is 
already private).

  112 void PeriodicTask::enroll() {
  113   // Follow normal safepoint aware lock enter protocol if the 
caller does
  114   // not already own the PeriodicTask_lock. Otherwise, we don't try to
  115   // enter it again to avoid deadlocking with a safepoint that started
  116   // after the initial enter and before this enter.
  117   //
  118   MutexLockerEx ml(PeriodicTask_lock->owned_by_self() ? NULL
  119                                                       : 
PeriodicTask_lock);

The deadlock is not with a safepoint interaction but the normal 
self-deadlock that would occur trying to lock() a Monitor/Mutex that you 
already own. (Monitors/Mutexes do not support recursive locking - in 
contrast to ObjectMonitors.) Ditto for disenroll. The original comment 
is the correct one with respect to acquiring with a safepoint check if 
not already locked:

/* XXX could be called from a JavaThread, so we have to check for
  * safepoint when taking the lock to avoid deadlocking */

---

thread.cpp:

1198 int WatcherThread::sleep() const {
1199   // The WatcherThread does not honor the safepoint protocol for
1200   // PeriodicTask_lock in order to provide more consistent task
1201   // execution timing.

Not sure that is really the correct characterisation. The WT doesn't 
need to honor the safepoint protocol because it isn't a JavaThread and 
so won't block even if a safepoint is in progress. So checking for a 
safepoint is simply a waste of time.

3565     MutexLocker ml(PeriodicTask_lock);

Why did you allow for a safepoint check here? It would only be necessary 
if a JavaThread might concurrently acquire the PeriodicTask_lock and a 
safepoint is initiated - during VM initialization. Not completely 
impossible I guess but have never heard of this occurring, so would 
require some other startup change to trigger it.

Thanks,
David
-----

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