RFR(L) 8153224 Monitor deflation prolong safepoints
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Apr 15 17:12:29 UTC 2019
Just tracking to make sure I made all the changes...
On 3/29/19 3:18 PM, Daniel D. Daugherty wrote:
> On 3/29/19 2:48 PM, Carsten Varming wrote:
>>
>>>> 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.
Done.
>
>>>> 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
Done.
Okay, I believe I've made all the changes for this set of comments
and replies...
Dan
More information about the hotspot-runtime-dev
mailing list