RFR(L) 8153224 Monitor deflation prolong safepoints
Carsten Varming
varming at gmail.com
Fri Mar 29 18:48:18 UTC 2019
Dear Dan,
On Fri, Mar 29, 2019 at 1:37 PM Daniel D. Daugherty <
daniel.daugherty at oracle.com> wrote:
> 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.
>
I read what you put on the wiki. Looked great to me. I'll scan it one more
time.
> 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> 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).
>
Nice.
> 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.
>
SGTM.
> 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.
Thanks,
Carsten
More information about the hotspot-runtime-dev
mailing list