RFR(L) 8153224 Monitor deflation prolong safepoints
Carsten Varming
varming at gmail.com
Fri Mar 29 23:58:09 UTC 2019
On Fri, Mar 29, 2019 at 3:19 PM Daniel D. Daugherty <
daniel.daugherty at oracle.com> wrote:
> 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> 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...
>
Wiki looks great to me.
> 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.
>
>
> This will be in the next round of code review.
>
Nice. I'll look forward to the next round.
>
>
>> 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.
>
Nice.
Carsten
More information about the hotspot-runtime-dev
mailing list