RFR(L) 8153224 Monitor deflation prolong safepoints

[Daniel D. Daugherty]

On 4/3/19 3:04 AM, Robbin Ehn wrote:
> Hi Dan, Carsten,
>
>>>> 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.
>>>>
>
> If I got the context correct here:
> All cleanups are done before the VM op is execute.
> It is the last thing we do in SS::begin(), so anything added to 
> SS::do_cleanup_tasks/ParallelSPCleanupTask is done first in any 
> safepoint.

Agreed.

src/hotspot/share/runtime/safepoint.cpp:

void SafepointSynchronize::begin() {
<snip>
   // We do the safepoint cleanup first since a GC related safepoint
   // needs cleanup to be completed before running the GC op.
   EventSafepointCleanup cleanup_event;
   do_cleanup_tasks();
   post_safepoint_cleanup_event(cleanup_event, _safepoint_counter);

   post_safepoint_begin_event(begin_event, _safepoint_counter, 
nof_threads, _current_jni_active_count);
   SafepointTracing::cleanup();
}

And do_cleanup_tasks() is where we do the deflate idle monitors work.

Dan



>
> /Robbin
>
>>>
>>> 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
>>