RFR(L) 8153224 Monitor deflation prolong safepoints
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Mar 29 19:18:47 UTC 2019
On 3/29/19 2:48 PM, Carsten Varming wrote:
> Dear Dan,
>
> On Fri, Mar 29, 2019 at 1:37 PM Daniel D. Daugherty
> <daniel.daugherty at oracle.com <mailto: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.
Thanks. I look forward to any feedback...
> 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).
>
>
> Nice.
This will be in the next round of code review.
> 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.
Good. This will be in the next round of code review.
Dan
>
> Thanks,
> Carsten
More information about the hotspot-runtime-dev
mailing list