RFR(L) 8153224 Monitor deflation prolong safepoints

Daniel D. Daugherty daniel.daugherty at oracle.com
Mon Apr 15 17:12:29 UTC 2019


Just tracking to make sure I made all the changes...


On 3/29/19 3:18 PM, Daniel D. Daugherty wrote:
> On 3/29/19 2:48 PM, Carsten Varming wrote:
>>
>>>>         I am trying to figure out when the global list of monitors,
>>>>         i.e., the monitors from dead threads, are deflated after a
>>>>         System.gc request. It looks like
>>>>         ObjectSynchronizer::do_safepoint_work should be responsible
>>>>         for this, but it calls deflate_idle_monitors if
>>>>         !AsyncDeflateIdleMonitors only. Perhaps I am missing something.
>>>
>>>         So System.gc() results in call to JVM_GC() here:
>>>
>>>         http://cr.openjdk.java.net/~dcubed/8153224-webrev/3-for-jdk13/src/hotspot/share/prims/jvm.cpp.udiff.html
>>>
>>>             The addition here is:
>>>
>>>             > ObjectSynchronizer::set_is_cleanup_requested(true);
>>>
>>>         which sets a flag. For per-thread lists:
>>>
>>>          void
>>>         ObjectSynchronizer::deflate_thread_local_monitors(Thread*
>>>         thread, DeflateMonitorCounters* counters) {
>>>         assert(SafepointSynchronize::is_at_safepoint(), "must be at
>>>         safepoint");
>>>
>>>         +  if (AsyncDeflateIdleMonitors) {
>>>         +    // Nothing to do when idle ObjectMonitors are deflated
>>>         using a
>>>         +    // JavaThread unless a special cleanup has been requested.
>>>         +    if (!is_cleanup_requested()) {
>>>         +      return;
>>>         +    }
>>>         +  }
>>>
>>>         we normally bail if AsyncDeflateIdleMonitors, but we do not
>>>         if is_cleanup_requested() is true so that takes care of
>>>         per-thread lists for a System.gc() call.
>>>
>>>         So here's deflate_idle_monitors() which handles the global
>>>         lists:
>>>
>>>         2007 void
>>>         ObjectSynchronizer::deflate_idle_monitors(DeflateMonitorCounters*
>>>         counters) {
>>>         2008 assert(!AsyncDeflateIdleMonitors, "sanity check");
>>>
>>>         so clearly that function can't be called (currently).
>>>
>>>         Stepping back for a second, a System.gc() will result in
>>>         a safepoint which will result in this call:
>>>
>>>              if
>>>         (_subtasks.try_claim_task(SafepointSynchronize::SAFEPOINT_CLEANUP_DEFLATE_MONITORS))
>>>         {
>>>                const char* name = "deflating global idle monitors";
>>>                EventSafepointCleanupTask event;
>>>                TraceTime timer(name, TRACETIME_LOG(Info, safepoint,
>>>         cleanup));
>>>         - ObjectSynchronizer::deflate_idle_monitors(_counters);
>>>         +      // AsyncDeflateIdleMonitors only uses
>>>         DeflateMonitorCounters
>>>         +      // when a special cleanup has been requested.
>>>         +      // Note: This logging output will include global idle
>>>         monitor
>>>         +      // elapsed times, but not global idle monitor
>>>         deflation count.
>>>         +
>>>         ObjectSynchronizer::do_safepoint_work(!AsyncDeflateIdleMonitors
>>>         ? _counters : NULL);
>>>
>>>         do_safepoint_work() will do the following:
>>>
>>>         1691   _gOmShouldDeflateIdleMonitors = true;
>>>         1692   MutexLockerEx ml(Service_lock,
>>>         Mutex::_no_safepoint_check_flag);
>>>         1693 Service_lock->notify_all();
>>>
>>>         which will cause the ServiceThread to deflate global idle
>>>         monitors
>>>         _sometime_ after the safepoint is complete.
>>>
>>>         So a System.gc() only causes deflate_thread_local_monitors() to
>>>         deflate per-thread idle monitors at the safepoint. The global
>>>         idle monitors are only handled by the ServiceThread.
>>>
>>>         Looking back through my notes when I added is_cleanup_requested
>>>         support, I only had a test case that had a monitor on a
>>>         per-thread
>>>         list that needed to be deflated when System.gc() is called.
>>>
>>>         I could change things so that deflate_idle_monitors() is also
>>>         called when is_cleanup_requested is true. That way a System.gc()
>>>         results in all monitor lists being cleaned at a safepoint which
>>>         is more consistent.
>>>
>>>         Carsten, would you like me to make this change?
>>>
>>>
>>>     I think it would be best to call deflate_idle_monitors() after a
>>>     System.gc() call before the actual garbage collection. If I
>>>     remember correctly, the monitors are GC roots, so not deflating
>>>     idle monitors would keep garbage alive contrary to the intended
>>>     meaning of System.gc().
>>
>>     I made the change to call deflate_idle_monitors() when
>>     is_cleanup_requested
>>     is true couple of days ago and I've been running it through some
>>     testing
>>     (so far with no new issues).
>>
>>
>> Nice.
>
> This will be in the next round of code review.

Done.


>
>>>>         Speaking of optimizations, it sure would be nice if little
>>>>         changes to java threads could be combined and performed on
>>>>         the way out of the safepoint in one go instead of having
>>>>         lots of iterations of the thread list in various places.
>>>>         Some people have thousands of threads and each traversal of
>>>>         the thread list hurts.
>>>
>>>         Do you have a specific example in mind?
>>>
>>>
>>>     No concrete example for a public mailing list. :(. But do notice
>>>     that independent tasks that require traversals of the thread
>>>     list are already fused in ParallelSPCleanupThreadClosure
>>>     <http://hg.openjdk.java.net/jdk-updates/jdk12u/file/fe6c633292ff/src/hotspot/share/runtime/safepoint.cpp#l586>.
>>>     If you made deflate_thread_local_monitors
>>>     set jt->omShouldDeflateIdleMonitors to true, then you wouldn't
>>>     need to iterator over all java threads in do_safepoint_work.
>>
>>     I think I see what you mean... so when
>>     ParallelSPCleanupThreadClosure::
>>     do_thread() calls deflate_thread_local_monitors():
>>
>>     2250   if (AsyncDeflateIdleMonitors) {
>>     2251     // Nothing to do when idle ObjectMonitors are deflated
>>     using a
>>     2252     // JavaThread unless a special cleanup has been requested.
>>
>>     Replace L2251-2 with:
>>              // Mark the JavaThread for idle monitor cleanup unless a
>>              // special cleanup has been requested.
>>     2253     if (!is_cleanup_requested()) {
>>
>>     Add these three lines:
>>                if (thread->omInUseCount > 0) {
>>                  // This JavaThread is using monitors so mark it.
>>                  thread->omShouldDeflateIdleMonitors = true;
>>                }
>>     2254       return;
>>     2255     }
>>
>>     That will allow this block to go away:
>>
>>     1695   // Request deflation of per-thread idle monitors by each
>>     JavaThread:
>>     1696   for (JavaThreadIteratorWithHandle jtiwh; JavaThread *jt =
>>     jtiwh.next(); ) {
>>     1697     if (jt->omInUseCount > 0) {
>>     1698       // This JavaThread is using monitors so check it.
>>     1699       jt->omShouldDeflateIdleMonitors = true;
>>     1700     }
>>     1701   }
>>
>>     Please let me know if I understand what you meant...
>>
>>
>> This is exactly what I meant.
>
> Good. This will be in the next round of code review 

Done.


Okay, I believe I've made all the changes for this set of comments
and replies...

Dan


More information about the hotspot-runtime-dev mailing list