RFR: 8244340: Handshake processing thread lacks yielding

[Daniel D. Daugherty]

 > v3 full:
 > http://cr.openjdk.java.net/~rehn/8244340/v3/full/

First, this is way more complicated than I was expecting. I've read all
the other code review replies so I _think_ I understand _why_ you went
this way, but I don't yet feel like I understand the _how_ of what you've
done here.

src/hotspot/share/runtime/handshake.hpp
     No comments.

src/hotspot/share/runtime/handshake.cpp
     L66: class HandshakeSpinYield : public StackObj {
         It would be good to have a short paragraph to explain the high 
lights
         of your handshake spin yield algorithm.

     L212:     HandshakeState::ProcessResult pr = 
HandshakeState::_no_operation;
         nit - extra space after the '='

     L213:     HandshakeSpinYield cbo(start_time_ns);
     L260:     HandshakeSpinYield cbo(start_time_ns);
     L351:   HandshakeSpinYield cbo(start_time_ns);
         In HotSpot, helper variables are usually named after the helper 
class
         so 'cbo' would be 'hsy'. Of course, I have to wonder what 'cbo' 
stands for?

src/hotspot/share/runtime/thread.hpp
     No comments.


Okay, I just re-read all the other code review comments for the third time.
I still don't understand your HandshakeSpinYield algorithm. Maybe it is just
me and I'm not seeing something obvious because my head is still stuck in
async monitor deflation land... :-)

Dan



On 5/7/20 3:03 AM, Robbin Ehn wrote:
> Hi Patricio,
>
> On 2020-05-06 18:24, Patricio Chilano wrote:
>>
>> On 5/6/20 5:53 AM, Robbin Ehn wrote:
>>> Hi Patricio,
>>>
>>> Here is v2 inc:
>>> http://cr.openjdk.java.net/~rehn/8244340/v2/inc/
>>> Full:
>>> http://cr.openjdk.java.net/~rehn/8244340/v2/full/
>> Thanks Robbin, looks good!
>
> Thanks!
>
> But I notice that when we do direct handshakes with a JavaThread we
> should yield in blocked state. This only effects the corner case when
> JavaThread A handshakes B and C handshakes A, C can complete the
> handshake faster if A would yield in block state.
>
> v3 inc:
> http://cr.openjdk.java.net/~rehn/8244340/v3/inc/
>
> v3 full:
> http://cr.openjdk.java.net/~rehn/8244340/v3/full/
>
> Thanks, Robbin
>
>>
>>
>> Patricio
>>> On 2020-05-05 21:44, Patricio Chilano wrote:
>>>> Hi Robbin,
>>>>
>>>> Thanks for fixing this!
>>>
>>> Thanks!
>>>
>>>>
>>>> I tested the patch in a Windows Server 2012 virtual machine (not 
>>>> VirtualBox), with two virtual cpus assigned. Without the patch, 
>>>> running the simple HelloWorld program takes about 2.5seconds most 
>>>> of the times when setting “/affinity 1” and around 300ms when 
>>>> setting “affinity /FF”. With the patch I get the same timing for 
>>>> both cases, about 300ms. It would be great if Miklos can also check 
>>>> it against his environment to see if his issue got fixed.
>>>
>>> Great. They don't seem to be building the JDK them self.
>>>
>>>>
>>>> Only small comments in handshake.cpp:
>>>> - In VM_HandshakeOneThread, last log_handshake_info() should be (pr 
>>>> == HandshakeState::_success) instead of by_vm_thread. Maybe just 
>>>> define "HandshakeState::ProcessResult pr" instead of by_vm_thread. 
>>>> Same with Handshake::execute_direct() to get rid of by_handshaker.
>>>
>>> Fixed.
>>>
>>>> - In Handshake::execute_direct() inside the "if" statement we could 
>>>> just break after verifying _success.
>>>
>>> The if statement is removed.
>>>
>>>> - I would remove _spin_time_ns and just keep _max_spin_time_ns, 
>>>> since both are only set at construction only.
>>>
>>> Fixed.
>>>
>>>> - I think a better name for _last_wait_ns should be 
>>>> last_spin_start_ns because it is updated also when we see the state 
>>>> changed.
>>>
>>> Fixed.
>>>
>>> Thanks, Robbin
>>>
>>>>
>>>> Thanks Robbin!
>>>>
>>>> Patricio
>>>> On 5/5/20 10:08 AM, Robbin Ehn wrote:
>>>>> Hi all, please review.
>>>>>
>>>>> As the issue describes there are problems with this infinitely 
>>>>> spinning.
>>>>> To avoid any performance regression we do not yield while we do or 
>>>>> see
>>>>> progress. But when we enter a period of no progression we 
>>>>> 'quickly' give
>>>>> up.
>>>>>
>>>>> Issue:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8244340
>>>>>
>>>>> Code:
>>>>> http://cr.openjdk.java.net/~rehn/8244340/v1/webrev/
>>>>>
>>>>> In the pathologically case we can easily see 10x improvement 
>>>>> starting the VM.
>>>>> I have found no performance regressions or performance gains in our
>>>>> standard benchmark setups.
>>>>> But I see much better average completion times for handshakes.
>>>>> For example specJVM2008 serial with ZGC shows 4x faster time to
>>>>> completion for handshakes in my setup, but little or no effect on 
>>>>> score
>>>>> (ops/m).
>>>>>
>>>>> Passes t1-3.
>>>>>
>>>>> Thanks, Robbin
>>>>
>>