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

make MutexLocker smarter about non-JavaThreads

    Details

    • Type: Enhancement
    • Status: Resolved
    • Priority: P3
    • Resolution: Fixed
    • Affects Version/s: 9
    • Fix Version/s: 13
    • Component/s: hotspot
    • Subcomponent:
    • Resolved In Build:
      b19
    • CPU:
      generic
    • OS:
      generic

      Description

      During the code review cycle for the following bug fix:

          JDK-8072439 fix for 8047720 may need more work

      Markus had an idea for making MutexLocker smarter when it is
      being used by non-JavaThreads. Here's the e-mail conversation:

      On 2/24/15 2:33 PM, Daniel D. Daugherty wrote:
      > More replies embedded below...
      >
      > On 2/24/15 10:02 AM, Markus Gronlund wrote:
      >>
      >> Actually thinking about this a bit more:
      >>
      >>
      >>
      >> I think we could make all uses of PeriodicTask_lock to be acquired with
      >> MutexLocker (not Ex), and avoid passing the Mutex::_no_safepoint_check
      >> flag (and lengthy comments):
      >>
      >>
      >>
      >> JavaThreads will (check for and) block for safepoints in WatcherThread::enroll/disenroll
      >> if the PeriodicTask_lock is being held by someone else. Same thing in before_exit().
      >>
      >>
      >>
      >> Since the WatcherThread is not a JavaThread and will never check for a safepoint if
      >> there is a contended lock, it will call IWait() (to park) directly.
      >>
      >>
      >>
      >> This would also make it possible to change the PeriodicTask_lock from being asserted
      >> as a "_safepoint_check_sometimes" to a "_safepoint_check_always".
      >
      > Coleen (and Max) would like that... :-)
      >
      >> In order to do this however, we would need to rework Monitor::Wait():
      >>
      >>
      >>
      >> The only place (currently) where there is a requirement to pass "Mutex::_no_safepoint_check"
      >> is when the WatcherThread calls Wait() - but this is because we have this in there:
      >>
      >>
      >>
      >> // !no_safepoint_check logically implies java_thread
      >>
      >> guarantee(no_safepoint_check || Self->is_Java_thread(), "invariant");
      >>
      >>
      >>
      >> This does not make sense - a WatcherThread should not need to explicitly say "please go
      >> outside the safepoint protocol" - it is not a JavaThread so to it, there is no such thing as a
      >> safepoint.
      >
      > That's why MutexLockerEx exists... but I see your point.
      > We're evolving this area with the _safepoint_check_* stuff
      > so why not make MutexLocker smarter...
      >
      >> In Monitor::lock() we branch to a safepoint check based on the Self->isJavaThread(), but in
      >> Monitor::wait() we also allow for JavaThreads to circumvent the protocol if they pass in the
      >> correct flag.
      >
      > This is definitely a little inconsistent.
      >
      >> Maybe we can change Monitor::Wait() a wee bit (I know this is sensitive code), and still allow
      >> for arbitrary passings of "no_safepoint_checks" for JavaThreads, but if there is nothing passed,
      >> we take the safepoint route if there is a JavaThread, and not if there is anything else (similar
      >> to Monitor::lock()). Callers which are not JavaThreads should not need to pass these options.
      >> Combining this with the lock assertion states, such as, "_safepoint_check_always" will disallow
      >> anyone (any JavaThread) to circumvent the safepoint protocol for the PeriodicTask_lock.
      >
      > Yes I agree this can likely be cleaned up a bit...
      >
      >> I will try some experiments, so Dan please go ahead with what you already have.
      >
      > So I'll leave the further cleanup to your experiment
      > and a new bug. I'll move forward with the webrev plus
      > the tweaks I identified in my reply to your first e-mail.
      >
      > Thanks again for the reviews!
      >
      > Dan
      >
      >>
      >>
      >> Cheers
      >>
      >> Markus

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                coleenp Coleen Phillimore
                Reporter:
                dcubed Daniel Daugherty
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: