Relates :
|
|
Relates :
|
|
Relates :
|
During the code review cycle for the following bug fix: JDK-8072439 fix for 8047720 may need more work it was mentioned that the PeriodicTask and WatcherThread APIs could be cleaned up further. >On 2/17/15 5:53 PM, David Holmes wrote: >> --- >> >> 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). > > I was too focused on adding asserts that enforced how it works today > and not on how it might be used down the road. There's no reason to > restrict the caller of time_to_wait() to the WatcherThread. I've > deleted the assert on line 82 and I added a comment to task.hpp: > > // Requires the PeriodicTask_lock. > static int time_to_wait(); > > By leaving time_to_wait() public, that allows code other than the > WatcherThread to use it perhaps for debugging... Okay. I still think the API is somewhat confused about its public interface - execute_if_pending should be protected for use by WT only (and subclasses implement it of course). Some cleanup was done via JDK-8072439, but another pass through both PeriodicTask and WatcherThread should be done: - look at public versus private; don't forget that 'friend' status grants access to 'private' methods... - look at whether the calling thread should be restricted (or not), e.g., WatcherThread::stop() should never be called by the WatcherThread itself - enroll() and disenroll() should probably never be called by the WatcherThread; what about other non-JavaThreads? David H. will probably have more ideas here.
|