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

[Stefan Karlsson]

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!

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