RFR(L) 8153224 Monitor deflation prolong safepoints
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Mar 29 17:37:31 UTC 2019
Hi Carsten,
One thing I forgot to ask is whether you had a chance to go through the
new OpenJDK wiki for Async Monitor Deflation:
> https://wiki.openjdk.java.net/display/HotSpot/Async+Monitor+Deflation
It would be good to have you and Roman review that wiki since you guys
did the original JEP: https://bugs.openjdk.java.net/browse/JDK-8183909.
If you and Roman agree that the OpenJDK wiki is an accurate description
of the project, then we should talk about withdrawing the JEP and moving
forward with the RFE: https://bugs.openjdk.java.net/browse/JDK-8143224.
More below...
On 3/29/19 1:04 PM, Carsten Varming wrote:
> Dear Dan,
>
> Please see inline.
>
> On Wed, Mar 27, 2019 at 2:56 PM Daniel D. Daugherty
> <daniel.daugherty at oracle.com <mailto:daniel.daugherty at oracle.com>> wrote:
>
> Hi Carsten,
>
> Thanks for the review.
>
>
> You are welcome.
>
> I've added back hotspot-runtime-dev at ... since this is the OpenJDK
> review phase...
>
>
> Oops. Sound good.
>
>
> 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?
>
>
> 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).
However, moving deflate_idle_monitors() from the safepoint cleanup phase
to before the actual garbage collection can wait until we do the work to
decouple triggering of monitor deflation to be independent of the the
safepoint cleanup phase.
>> 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.
>
>
> Great.
>
>> 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...
Dan
>
> Again, thanks for the review!
>
>
> You are welcome,
> Carsten
>
> 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.
>>
>
More information about the hotspot-runtime-dev
mailing list