RFR(L) 8153224 Monitor deflation prolong safepoints
Carsten Varming
varming at gmail.com
Fri Mar 29 17:04:52 UTC 2019
Dear Dan,
Please see inline.
On Wed, Mar 27, 2019 at 2:56 PM Daniel D. Daugherty <
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().
> 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.
> 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> 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