RFR: 7143664: Clean up OrderAccess implementations and usage

[David Holmes]

On 29/01/2015 8:48 AM, David Holmes wrote:
> Updated webrev:
>
> http://cr.openjdk.java.net/~dholmes/7143664/webrev.v2/

First let me say that Erik and I had a lot of offline discussions about 
this since he first proposed his reworking. My initial concern was 
ensuring the model changes were appropriate as this is a complex area 
and there can be different views of how things can/should be expressed. 
As per the bug report I was aware of a number of issues with the 
existing expression of semantics, but Erik showed me things were even 
worse than I had thought :)

Basically the definitions of stand-alone release() and acquire() as they 
currently are in orderAccess.hpp are somewhat broken. "release" 
semantics must establish a barrier between accesses prior to the 
release() and a specific store after the release - hence release_store() 
and similarly for acquire() and a preceding load, hence load_acquire(). 
The general definitions of acquire() and release() as one-way movement 
barriers don't provide useful semantics - in part because regardless of 
which way you move things once everything is on the same side of the 
barrier you can't tell in which direction they moved. :)

Further, those general definitions would imply that release_store() and 
release();store are semantically very different - but they are currently 
implemented (as are things like release_store_fence) as if semantically 
the same.

And finally, while orderAccess.hpp suggests that release() can be 
implemented by a storeStore|loadStore barrier, there is code throughout 
the VM that expects it is exactly a storeStore|storeload barrier. (And 
similarly for acquire). C1 for example explicitly defines:

membar_acquire := LoadLoad + LoadStore
membar_release := LoadStore + StoreStore
membar         := StoreLoad (aka fence)

and if I then look at where "release" is used in the codebase they all 
seem incorrect based on "release" semantics, but correct if you expect 
release() to mean LoadStore|StoreStore (and in most cases you would then 
actually use a release_store).

So the changes Erik proposes to ensure that release() and acquire() are 
defined with respect to that subsequent-store or preceding-load, bring 
expectations in the code back into alignment. It means release(); store 
is the same as release_store() semantically, and the latter is a means 
for optimizing when you place the barrier and the store directly 
together - where often you can't because the actual store/load may be 
buried within other accessor methods eg:

OrderAccess::release();
java_lang_Thread::set_xxx(threadobj, value);

We can fix that by adding set_with_release/load_with_acquire, or passing 
in barrier enums C++11 style, but that is a lot of rework.

On the implementation side I'm a template novice and was originally 
skeptical about the technique, but having implemented it for a couple of 
platforms I'm now a fan - even if I couldn't walk you through the exact 
behaviour of the template specialization code :)

So on to the actual review comments ...

---

src/share/vm/runtime/orderAccess.hpp

Minor nit: I would prefer the term "bound" rather than "joined" when 
referring to load_acquire and store_release.

In the implementation table I think "sparc" should be "sparc TSO".

I still think we need to say something about C++ volatile and about 
compiler barriers in general.

---

src/share/vm/runtime/orderAccess.inline.hpp

No comment

---

src/cpu/x86/vm/c1_LIRAssembler_x86.cpp

No comment (this just removes stale/unused terminology)

---

src/os_cpu/aix_ppc/vm/orderAccess_aix_ppc.inline.hpp
src/os_cpu/linux_ppc/vm/orderAccess_linux_ppc.inline.hpp

No comments - ppc64 folk need to review these.

---

src/os_cpu/bsd_x86/vm/orderAccess_bsd_x86.inline.hpp
src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp

In OrderAccess::fence doesn't the existing asm already act as a compiler 
barrier; and on uniprocessor we need neither compiler nor hardware 
barriers; so isn't the compiler_barrier() redundant?

---

src/os_cpu/bsd_zero/vm/orderAccess_bsd_zero.inline.hpp
src/os_cpu/linux_zero/vm/orderAccess_linux_zero.inline.hpp

No comments - The Zero folk will need to approve this change in terminology.

---

src/os_cpu/linux_sparc/vm/orderAccess_linux_sparc.inline.hpp

No comments.

---

src/os_cpu/solaris_sparc/vm/orderAccess_solaris_sparc.inline.hpp
src/os_cpu/solaris_sparc/vm/solaris_sparc.il

You've removed the GNU_SOURCE ifdefs which means the new code has to be 
tested using gcc on Solaris. We don't officially support that platform 
so I have no way to verify these particular changes.

---

src/os_cpu/solaris_x86/vm/orderAccess_solaris_x86.inline.hpp
src/os_cpu/solaris_x86/vm/solaris_x86_32.il
src/os_cpu/solaris_x86/vm/solaris_x86_64.il

I think it makes sense to use the same code here as the linux x86 
version. But as above can't verify this for gcc on Solaris; also query 
about need for compiler_barrier() in fence().

---
  src/os_cpu/windows_x86/vm/orderAccess_windows_x86.inline.hpp

Same comment about fence() and compiler_barrier() :)

Do you have a reference to the MSVC volatile semantics?

Not clear about the 32-bit versus 64-bit changes - mainly because I 
don't know why they were different in the first place.

Why are windows release_store_fence specializations different to linux 
x86 versions?

Not clear about your change to the float version of release_store_fence:
a) do we need to change it?
b) given what we changed it to isn't that the default ?

---

General comment: copyright dates need updating in most, if not all, files.

Thanks,
David