RFR: 8244340: Handshake processing thread lacks yielding

[Robbin Ehn]

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