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

[David Holmes]

Hi Martin,

On 2/07/2018 4:14 PM, Doerr, Martin wrote:
> Hi David,
> 
> thank you for your support. Please let me know if your testing has passed successfully. I didn't get a reply from submit11, but our internal builds and some tests were run.

Yes all good - only one unrelated failure.

Cheers,
David

> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Montag, 2. Juli 2018 01:58
> To: Doerr, Martin <martin.doerr at sap.com>; Erik Österlund <erik.osterlund at oracle.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
> 
> Hi Martin,
> 
> On 29/06/2018 8:12 PM, Doerr, Martin wrote:
>> 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/
> 
> That all seems fine.
> 
>> Due to this change, SafepointMechanism::initialize_header doesn't use a release barrier anymore which should be fine.
> 
> I agree. The JavaThread being constructed does not yet have a native
> thread associated with it so there is no "acquire" for a "release" to
> pair with in this case. Native thread creation/execution has its own
> memory barriers.
> 
>> Pushed to jdk/submit11 and our internal testing.
> 
> I'll put this through our internal testing too.
> 
> Thanks,
> David
> 
>>
>> 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
>>>>
>>>