RFR: 8244340: Handshake processing thread lacks yielding

[David Holmes]

Hi Robbin,

Thanks for the additional explanations in other email. I still think we 
have some scope for ensuring the different "spin" approaches are consistent.

On 7/05/2020 5:03 pm, 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.

I'm a bit surprised you chose to do this - the thread-state transition 
logic would likely exceed the duration of these microsleeps and end up 
executing even more code in the thread we want to go off-proc. That's 
why these sleep routines don't themselves change the thread state etc.

I also can't quite get my head around what kind of nested processing 
this can now lead to (with regard to safepoints and other handshakes on 
the thread doing this handshake).

Thanks,
David
-----

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