RFR(S): 8206003: SafepointSynchronize with TLH: StoreStore barriers should be moved out of the loop

[Doerr, Martin]

Thank you for the reviews.

I've created a new webrev with a "_release" version instead of "_no_release":
http://cr.openjdk.java.net/~mdoerr/8206003_tlh_sync_membars/webrev.01/

Due to this change, SafepointMechanism::initialize_header doesn't use a release barrier anymore which should be fine.

Pushed to jdk/submit11 and our internal testing.

Best regards,
Martin


-----Original Message-----
From: David Holmes [mailto:david.holmes at oracle.com] 
Sent: Freitag, 29. Juni 2018 00:49
To: Erik Österlund <erik.osterlund at oracle.com>; Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-dev at openjdk.java.net; Robbin Ehn <robbin.ehn at oracle.com>; Andrew Haley (aph at redhat.com) <aph at redhat.com>
Subject: Re: RFR(S): 8206003: SafepointSynchronize with TLH: StoreStore barriers should be moved out of the loop

On 29/06/2018 1:28 AM, Erik Österlund wrote:
> Hi Martin,
> 
> This did catch my eye too. This looks good to me. But could you consider 
> having _release in the name of the setter that uses release, and no 
> postfix for the one using a plain store, instead of giving that one a 
> _no_release postfix. I don't need another webrev.

+1

I'm assuming that nothing may be tripped up (ie assertion somewhere) if 
the polling status of different threads can now be seen out-of-order.

Thanks,
David

> 
> Thanks,
> /Erik
> 
> On 2018-06-28 16:52, Doerr, Martin wrote:
>> Hi,
>>
>> I have recently come across a bad placement of memory barriers in 
>> SafepointSynchronize::begin() and end() which were changed for JEP 
>> 312: Thread-Local Handshakes. They iterate over all JavaThreads and 
>> call SafepointMechanism::arm_local_poll or disarm_local_poll. 
>> Unfortunately, the release barriers are inside the latter functions.
>>
>> Assume we have several 1000 JavaThreads. This means the code executes 
>> several 1000 release barriers on weak memory model platforms (PPC64 
>> and ARM/aarch64). Only one is needed.
>>
>> A goal of JEP 312 was to minimize latency of safepoints which gets 
>> defeated by this issue to some extend on these platforms.
>>
>> It could be fixed by this proposal:
>> http://cr.openjdk.java.net/~mdoerr/8206003_tlh_sync_membars/webrev.00/
>>
>> Please review.
>>
>> Best regards,
>> Martin
>>
>