RFR (M): 8245721: Refactor the TaskTerminator

[Thomas Schatzl]

Hi Zhengyu,

   thanks for your review.

On 26.06.20 15:25, Zhengyu Gu wrote:
> Hi Thomas,
> 
> I believe you can use MonitorLocker (vs. MutexLocker) to remove naked 
> _blocker->waitxxxx.

I plan to drop the for-Java Monitor and use os::PlatformMonitor instead 
here. That removes all _no_safepoint_check_flag code.

Also there is no MonitorUnlocker, so we'd still have to use 
MutexUnlocker. It will look a bit weird to not have matching 
locker/unlocker names.

Would it be okay for you to wait for that?

Thanks,
   Thomas

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