Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8074314

PeriodicTask and WatcherThread APIs need to be cleaned up



    • Enhancement
    • Status: Closed
    • P4
    • Resolution: Won't Fix
    • 9
    • tbd
    • hotspot
    • generic
    • generic


      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.


        Issue Links



              Unassigned Unassigned
              dcubed Daniel Daugherty
              0 Vote for this issue
              3 Start watching this issue