RFR: 8234562: Move OrderAccess::release_store*/load_acquire to Atomic

[David Holmes]

On 22/11/2019 8:55 pm, Stefan Karlsson wrote:
> Hi David,
> 
> On 2019-11-22 00:52, David Holmes wrote:
>> Hi Stefan,
>>
>> This generally all seems fine.
> 
> Thanks.
> 
>>
>> I have one concern about header file includes. There are not many 
>> changes here that change an include of orderAccess.hpp to atomic.hpp. 
>> So I think we may have missing includes, or at least have very 
>> indirect include paths. For example compiledMethod.cpp doesn't include 
>> atomic.hpp, nor does compiledMethod.inline.hpp, but the inline.hpp 
>> does include orderAccess.hpp which is no longer needed.
> 
> I agree. I thought about doing a full cleanup of this, but backed away, 
> because I didn't want to redo all builds on all platforms and configs
> 
> Would you be OK if I created a cleanup patch that can be pushed after 
> these changes? Last time I did a complete include cleanup it took over a 
> week to get that reviewed!

As long as it builds on all platforms with and without PCH. I don't want 
this to trigger build failures for anyone.

Thanks,
David
-----

>>
>> Aside: I see some odd looking uses of load_acquire/release_store 
>> pairings :(
>>
>> Minor nits:
>>
>> src/hotspot/os_cpu/aix_ppc/atomic_aix_ppc.hpp
>>
>> seems to be an indentation issue:
>>
>> +    T t = Atomic::load(p);
>> +    // Use twi-isync for load_acquire (faster than lwsync).
>> +    __asm__ __volatile__ ("twi 0,%0,0\n isync\n" : : "r" (t) : 
>> "memory");
>> +    return t;
>>
> 
> fixed
> 
>> ---
>>
>> src/hotspot/os_cpu/linux_ppc/atomic_linux_ppc.hpp
>>
>> seems to be an indentation issue:
>>
>> +    __asm__ __volatile__ ("twi 0,%0,0\n isync\n" : : "r" (t) : 
>> "memory");
>> +    return t;
>>
> 
> fixed
> 
>> ---
>>
>> src/hotspot/os_cpu/windows_x86/atomic_windows_x86.hpp
>>
>> +// bound calls like release_store go through OrderAccess::load
>> +// and OrderAccess::store which do volatile memory accesses.
>>
>> s/OrderAccess/Atomic/
>>
> 
> fixed
> 
>> I just realised this used to be technically correct given:
>>
>> class OrderAccess : private Atomic {
>>
>> but I personally never realized OrderAccess and Atomic were related 
>> this way! :)
>>
> 
> :)
> 
> Thanks for reviewing,
> StefanK
> 
>> ---
>>
>> Thanks,
>> David
>> -----
>>
>> On 21/11/2019 8:33 pm, Stefan Karlsson wrote:
>>> Hi all,
>>>
>>> I'd like to propose that we move release_store, release_store_fence, 
>>> and load_acquire, from OrderAccess to Atomic.
>>>
>>> https://cr.openjdk.java.net/~stefank/8234562/webrev.01/
>>> https://bugs.openjdk.java.net/browse/JDK-8234562
>>>
>>> Atomic already has the relaxed store and load, among other functions 
>>> for concurrent access, but release_store, release_store_fence, and 
>>> load_acquire, are located in OrderAccess.
>>>
>>> After this change there's an inconsistency in the order of the 
>>> parameters in the store functions in the Atomic API:
>>>
>>>   void store(T store_value, volatile D* dest)
>>>   void release_store(volatile D* dest, T store_value)
>>>   void release_store_fence(volatile D* dest, T store_value)
>>>
>>> I'd like to address that in a separate RFE, where I propose that we 
>>> move the dest parameter to the left for all the Atomic functions. See:
>>> https://bugs.openjdk.java.net/browse/JDK-8234563
>>>
>>> I've tested this patch with tier1-7. I've also built fastdebug on the 
>>> following configs: aarch64, arm32, ppc64le, s390x, shenandoah, zero, 
>>> minimal, just to minimize any disruptions.
>>>
>>> Thanks,
>>> StefanK