RFR (M): 8245721: Refactor the TaskTerminator

[Zhengyu Gu]

Hi Thomas,

I believe you can use MonitorLocker (vs. MutexLocker) to remove naked 
_blocker->waitxxxx.

diff -r d76db3e96d46 src/hotspot/share/gc/shared/taskTerminator.cpp
--- a/src/hotspot/share/gc/shared/taskTerminator.cpp    Fri Jun 26 
07:59:40 2020 -0400
+++ b/src/hotspot/share/gc/shared/taskTerminator.cpp    Fri Jun 26 
09:23:00 2020 -0400
@@ -153,7 +153,7 @@
    Thread* the_thread = Thread::current();
    SpinContext spin_context;

-  MutexLocker x(_blocker, Mutex::_no_safepoint_check_flag);
+  MonitorLocker x(_blocker, Mutex::_no_safepoint_check_flag);
    _offered_termination++;

    if (_offered_termination == _n_threads) {
@@ -194,7 +194,7 @@
        // Give up spin master before sleeping.
        _spin_master = NULL;
      }
-    _blocker->wait_without_safepoint_check(WorkStealingSleepMillis);
+    x.wait(WorkStealingSleepMillis);

      // Immediately check exit conditions after re-acquiring the lock.
      if (_offered_termination == _n_threads) {

-Zhengyu

On 6/24/20 4:03 AM, Thomas Schatzl wrote:
> Hi all,
> 
>    can I have reviews for this refactoring of the (OWST) TaskTerminator 
> to make the algorithm more understandable.
> 
> The original implementation imho suffers from two issues:
> 
> - manual lock() and unlock() of the _blocker synchronization lock 
> everywhere, distributed around two separate methods.
> 
> - interspersing the actual spinning code somewhere inlined inbetween.
> 
> This change tries to hopefully successfully make reasoning about the 
> code *much* easier by different separation of these two methods, and 
> using scoped locks.
> 
> The final structure of the code has been intensively tested to not cause 
> a regression in performance, however it made a few "obvious" further 
> refactorings undesired due to signficant perf regressions.
> 
> I believe I found a good tradeoff here, but I am of course open to 
> improvements :) I tried to sketch a few of those ultimately unsuccessful 
> attempts in the CR.
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8245721
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8245721/webrev/
> Testing:
> tier1-5, many many perf rounds, many tier1-X rounds with other patches
> 
> Thanks,
>    Thomas
>