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

David Holmes david.holmes at oracle.com
Thu Nov 21 23:52:30 UTC 2019

Hi Stefan,

This generally all seems fine.

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.

Aside: I see some odd looking uses of load_acquire/release_store pairings :(

Minor nits:


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;



seems to be an indentation issue:

+    __asm__ __volatile__ ("twi 0,%0,0\n isync\n" : : "r" (t) : "memory");
+	return t;



+// bound calls like release_store go through OrderAccess::load
+// and OrderAccess::store which do volatile memory accesses.


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! :)



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

More information about the hotspot-dev mailing list