--- old/src/share/vm/runtime/java.cpp Tue Feb 17 13:56:34 2015 +++ new/src/share/vm/runtime/java.cpp Tue Feb 17 13:56:34 2015 @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2014, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -422,9 +422,11 @@ os::infinite_sleep(); } - // Terminate watcher thread - must before disenrolling any periodic task - if (PeriodicTask::num_tasks() > 0) + // Stop the WatcherThread. We do this before disenrolling various + // PeriodicTasks to reduce the likelyhood of races. + if (PeriodicTask::num_tasks() > 0) { WatcherThread::stop(); + } // Print statistics gathered (profiling ...) if (Arguments::has_profile()) { --- old/src/share/vm/runtime/task.cpp Tue Feb 17 13:56:34 2015 +++ new/src/share/vm/runtime/task.cpp Tue Feb 17 13:56:34 2015 @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2014, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -47,6 +47,8 @@ #endif void PeriodicTask::real_time_tick(int delay_time) { + assert(Thread::current()->is_Watcher_thread(), "must be WatcherThread"); + #ifndef PRODUCT if (ProfilerCheckIntervals) { _ticks++; @@ -60,6 +62,9 @@ #endif { + // The WatcherThread does not honor the safepoint protocol for + // PeriodicTask_lock in order to provide more consistent task + // execution timing. MutexLockerEx ml(PeriodicTask_lock, Mutex::_no_safepoint_check_flag); int orig_num_tasks = _num_tasks; @@ -74,8 +79,8 @@ } int PeriodicTask::time_to_wait() { - MutexLockerEx ml(PeriodicTask_lock->owned_by_self() ? - NULL : PeriodicTask_lock, Mutex::_no_safepoint_check_flag); + assert(Thread::current()->is_Watcher_thread(), "must be WatcherThread"); + assert(PeriodicTask_lock->owned_by_self(), "PeriodicTask_lock required"); if (_num_tasks == 0) { return 0; // sleep until shutdown or a task is enrolled @@ -98,14 +103,20 @@ } PeriodicTask::~PeriodicTask() { + // This PeriodicTask may have already been disenrolled by a + // subclass with a non-trivial destructor. disenroll(); } -/* enroll could be called from a JavaThread, so we have to check for - * safepoint when taking the lock to avoid deadlocking */ +// enroll the current PeriodicTask void PeriodicTask::enroll() { - MutexLockerEx ml(PeriodicTask_lock->owned_by_self() ? - NULL : PeriodicTask_lock); + // Follow normal safepoint aware lock enter protocol if the caller does + // not already own the PeriodicTask_lock. Otherwise, we don't try to + // enter it again to avoid deadlocking with a safepoint that started + // after the initial enter and before this enter. + // + MutexLockerEx ml(PeriodicTask_lock->owned_by_self() ? NULL + : PeriodicTask_lock); if (_num_tasks == PeriodicTask::max_tasks) { fatal("Overflow in PeriodicTask table"); @@ -113,7 +124,7 @@ _tasks[_num_tasks++] = this; WatcherThread* thread = WatcherThread::watcher_thread(); - if (thread) { + if (thread != NULL) { thread->unpark(); } else { WatcherThread::start(); @@ -120,11 +131,15 @@ } } -/* disenroll could be called from a JavaThread, so we have to check for - * safepoint when taking the lock to avoid deadlocking */ +// disenroll the current PeriodicTask void PeriodicTask::disenroll() { - MutexLockerEx ml(PeriodicTask_lock->owned_by_self() ? - NULL : PeriodicTask_lock); + // Follow normal safepoint aware lock enter protocol if the caller does + // not already own the PeriodicTask_lock. Otherwise, we don't try to + // enter it again to avoid deadlocking with a safepoint that started + // after the initial enter and before this enter. + // + MutexLockerEx ml(PeriodicTask_lock->owned_by_self() ? NULL + : PeriodicTask_lock); int index; for(index = 0; index < _num_tasks && _tasks[index] != this; index++) --- old/src/share/vm/runtime/thread.cpp Tue Feb 17 13:56:35 2015 +++ new/src/share/vm/runtime/thread.cpp Tue Feb 17 13:56:34 2015 @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2014, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -1196,8 +1196,16 @@ } int WatcherThread::sleep() const { + // The WatcherThread does not honor the safepoint protocol for + // PeriodicTask_lock in order to provide more consistent task + // execution timing. MutexLockerEx ml(PeriodicTask_lock, Mutex::_no_safepoint_check_flag); + if (_should_terminate) { + // check for termination before we do any housekeeping or wait + return 0; // we did not sleep. + } + // remaining will be zero if there are no tasks, // causing the WatcherThread to sleep until a task is // enrolled @@ -1210,8 +1218,9 @@ jlong time_before_loop = os::javaTimeNanos(); - for (;;) { - bool timedout = PeriodicTask_lock->wait(Mutex::_no_safepoint_check_flag, remaining); + while (true) { + bool timedout = PeriodicTask_lock->wait(Mutex::_no_safepoint_check_flag, + remaining); jlong now = os::javaTimeNanos(); if (remaining == 0) { @@ -1252,7 +1261,7 @@ this->initialize_thread_local_storage(); this->set_native_thread_name(this->name()); this->set_active_handles(JNIHandleBlock::allocate_block()); - while (!_should_terminate) { + while (true) { assert(watcher_thread() == Thread::current(), "thread consistency check"); assert(watcher_thread() == this, "thread consistency check"); @@ -1288,6 +1297,11 @@ } } + if (_should_terminate) { + // check for termination before posting the next tick + break; + } + PeriodicTask::real_time_tick(time_waited); } @@ -1318,27 +1332,20 @@ } void WatcherThread::stop() { - // Get the PeriodicTask_lock if we can. If we cannot, then the - // WatcherThread is using it and we don't want to block on that lock - // here because that might cause a safepoint deadlock depending on - // what the current WatcherThread tasks are doing. - bool have_lock = PeriodicTask_lock->try_lock(); + { + // Follow normal safepoint aware lock enter protocol since the + // WatcherThread is stopped by another JavaThread. + MutexLocker ml(PeriodicTask_lock); + _should_terminate = true; + OrderAccess::fence(); // ensure WatcherThread sees update in main loop - _should_terminate = true; - OrderAccess::fence(); // ensure WatcherThread sees update in main loop - - if (have_lock) { WatcherThread* watcher = watcher_thread(); if (watcher != NULL) { - // If we managed to get the lock, then we should unpark the - // WatcherThread so that it can see we want it to stop. + // unpark the WatcherThread so it can see that it should terminate watcher->unpark(); } - - PeriodicTask_lock->unlock(); } - // it is ok to take late safepoints here, if needed MutexLocker mu(Terminator_lock); while (watcher_thread() != NULL) { @@ -1358,9 +1365,7 @@ } void WatcherThread::unpark() { - MutexLockerEx ml(PeriodicTask_lock->owned_by_self() - ? NULL - : PeriodicTask_lock, Mutex::_no_safepoint_check_flag); + assert(PeriodicTask_lock->owned_by_self(), "PeriodicTask_lock required"); PeriodicTask_lock->notify(); } @@ -3557,8 +3562,8 @@ } { - MutexLockerEx ml(PeriodicTask_lock, Mutex::_no_safepoint_check_flag); - // Make sure the watcher thread can be started by WatcherThread::start() + MutexLocker ml(PeriodicTask_lock); + // Make sure the WatcherThread can be started by WatcherThread::start() // or by dynamic enrollment. WatcherThread::make_startable(); // Start up the WatcherThread if there are any periodic tasks