RFR (M): 8245721: Refactor the TaskTerminator

[Zhengyu Gu]

On 6/26/20 10:26 AM, Thomas Schatzl wrote:
> 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.

Yes, sounds good idea.

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

Sure.

Thanks,

-Zhengyu

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