RFR(L) 8153224 Monitor deflation prolong safepoints

[Daniel D. Daugherty]

Hi Carsten,

Thanks for the review.

I've added back hotspot-runtime-dev at ... since this is the OpenJDK review 
phase...


On 3/27/19 2:08 PM, Carsten Varming wrote:
> Dear Dan,
>
> The code looks great.

Thanks! (Though much of it is yours :-))


> 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?


> Possible optimization: 
> ObjectSynchronizer::deflate_thread_local_monitors is executed 
> unconditionally in every safepoint on each java thread. What if 
> deflate_thread_local_monitors would set the flag 
> omShouldDeflateIdleMonitors to true if AsyncDeflateIdleMonitors and a 
> gc is not requested? This could eliminate the iteration over all 
> threads in do_safepoint_work. Perhaps this is worthy of a follow-up 
> change.

Robbin would like to see do_safepoint_work() get eliminated and
for us to come up with a different want to periodically cause
monitor deflation to be invoked for both AsyncDeflateIdleMonitors
and !AsyncDeflateIdleMonitors. So there are already plans to
remove this work from the safepoint mechanism. I don't know if
Robbin has a bug tracking this or not; I'll have to check.


> 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?

Again, thanks for the review!

Dan



>
> Thanks,
> Carsten
>
> On Sun, Mar 24, 2019 at 6:57 AM Daniel D. Daugherty 
> <daniel.daugherty at oracle.com <mailto:daniel.daugherty at oracle.com>> wrote:
>
>     Greetings,
>
>     Welcome to the OpenJDK review thread for my port of Carsten's work on:
>
>          JDK-8153224 Monitor deflation prolong safepoints
>     https://bugs.openjdk.java.net/browse/JDK-8153224
>
>     Here's a link to the OpenJDK wiki that describes my port:
>
>     https://wiki.openjdk.java.net/display/HotSpot/Async+Monitor+Deflation
>
>     Here's the webrev URL:
>
>     http://cr.openjdk.java.net/~dcubed/8153224-webrev/3-for-jdk13/
>
>     Here's a link to Carsten's original webrev:
>
>     http://cr.openjdk.java.net/~cvarming/monitor_deflate_conc/0/
>
>     Earlier versions of this patch have been through several rounds of
>     preliminary review. Many thanks to Carsten, Coleen, Robbin, and
>     Roman for their preliminary code review comments. A very special
>     thanks to Robbin and Roman for building and testing the patch in
>     their own environments (including specJBB2015).
>
>     This version of the patch has been thru Mach5 tier[1-8] testing on
>     Oracle's usual set of platforms. Earlier versions have been run
>     through my stress kit on my Linux-X64 and Solaris-X64 servers
>     (product, fastdebug, slowdebug).Earlier versions have run Kitchensink
>     for 12 hours on MacOSX, Linux-X64 and Solaris-X64 (product, fastdebug
>     and slowdebug). Earlier versions have run my monitor inflation stress
>     tests for 12 hours on MacOSX, Linux-X64 and Solaris-X64 (product,
>     fastdebug and slowdebug).
>
>     All of the testing done on earlier versions will be redone on the
>     latest version of the patch.
>
>     Thanks, in advance, for any questions, comments or suggestions.
>
>     Dan
>
>     P.S.
>     One subtest in gc/g1/humongousObjects/TestHumongousClassLoader.java
>     is currently failing in -Xcomp mode on Win* only. I've been trying
>     to characterize/analyze this failure for more than a week now. At
>     this point I'm convinced that Async Monitor Deflation is aggravating
>     an existing bug. However, I plan to have a better handle on that
>     failure before these bits are pushed to the jdk/jdk repo.
>