RFR: 8067790: Better support for native implementations of Atomic::cmpxchg

[Volker Simonis]

Hi Erik,

nice change (did I mention already that I LOVE template metaprogramming :)

first some minor layout stuff:

- some of your file contain trailing white space (mostly in empty
lines). Running jcheck on your change reports:

src/os_cpu/bsd_x86/vm/atomic_bsd_x86.hpp:33: Trailing whitespace
src/os_cpu/linux_x86/vm/atomic_linux_x86.hpp:33: Trailing whitespace
src/os_cpu/solaris_x86/vm/atomic_solaris_x86.hpp:33: Trailing whitespace
src/os_cpu/windows_x86/vm/atomic_windows_x86.hpp:33: Trailing whitespace
src/share/vm/utilities/traits/isBaseOf.cpp:32: Trailing whitespace
src/share/vm/utilities/traits/isBaseOf.hpp:63: Trailing whitespace
src/share/vm/utilities/traits/isClassOrUnion.hpp:35: Trailing whitespace
src/share/vm/utilities/traits/isSame.cpp:89: Trailing whitespace
src/share/vm/utilities/traits/selectBaseClass.hpp:39: Trailing whitespace

Could you please fix these lines and make your change 'jcheck'-safe.

- all your test functions are static except
IsBaseOfAndDerivedTest::test()  in isBaseOf.cpp. I understand that it
is not strictly necessary but for symmetric reasons you could make it
static as well.

- your test are elegant and I like them. However I wonder how we can
be sure that the compiler really instantiates all the templates. I can
see that there's no code created for the test functions in the object
files (which is fine). But how can we be sure that they are relly
compiled? Do you have any insights here? (By the way, I noticed that
if a insert a wrong STATIC_ASSERT I get a compile time warning, so
your tests work in general. I only wonder if they really work always
or if a smart compiler can just figure out that he doesn't need to
compile them?)

 - have you also done some performance checks (i.e. is the templatized
version really as fast as the one with macros?)

Following some additional platform/compiler checks (just in case
somebody is interested):

 - doesn’t compile with aCC A.03.73 on HP UX 11.11 / PARISC (SIGSEGV
in compiler)
 - compiles with aCC A.03.95.01 on HP UX 11.31 / PARISC (with
+hpxstd98, without +hpxstd98 SIGSEGV in compiler)

 - compiles with aCC A.06.16.01 on HP UX 11.23 / IA64

 - compiles with g++ 4.1.2 on Linux/ppc64

 - compiles with xlc 10.1 on AIX 5.3
 - compiles with xlC 12.01 on AIX 7.1

Regards,
Volker

On Wed, Dec 17, 2014 at 5:38 PM, Erik Österlund <erik.osterlund at lnu.se> wrote:
> Hi all,
>
> Previously, jbyte Atomic::cmpxchg had one general implementation for all platforms. A macro based solution was introduced, adding specialized implementation for the x86 platforms without changing code for the ports for other architectures, making the specialization optional (from ).
>
> However, macros are not very nice and this new fix gets rid of the macros and replaces it with a cleaner solution using inheritance and templates. The core idea is simple: split the Atomic class into three layers of inheritance.
>
> 1. AtomicBase is the base class where general operations for all platforms should go, now it contains the general implementation for jbyte cmpxchg, but in the future other similar methods are welcome too.
> 2. AtomicPlatform is an optional class that a platform can define, but does not have to (and hence does not break existing ports), allowing specialized variants to override the behaviour of AtomicBase. Now it contains the specialized platform dependent x86 variants of jbyte cmpxchg.
> 3. Atomic is the fascade of all atomics as expected. Its jbyte cmpxchg implementation simply forwards to its super class AtomicSuper which is either AtomicBase or AtomicPlatform if provided for the specific platform.
>
> All in all, it’s based on simple inheritance with the twist that the AtomicPlatform part is optional. This is allowed using some C++ metaprogramming idioms. Concretely, a trait was implemented called SelectBaseClass that allows you to specify the desired base class, and another one to fall back to in case the desired base class was not defined (forward declaration). The super class of Atomic is hence defined as:
>
> typedef SelectBaseClass<AtomicPlatform, AtomicBase>::type AtomicSuper;
>
> To implement this trait, an equivalent variant of the C++11/boost is_base_of trait was implemented, called IsBaseOf to follow our naming conventions. It is very similar to the standard version but works on all of our compilers. The only difference so far between IsBaseOf and is_base_of is that IsBaseOf will return true when comparing two unions that are the same, while is_base_of will not. The reason for this is that I found no portable way of detecting unions. A bunch of dependent traits were implemented too, and it can all be found in utilities/traits/ since I like reasonably fine grained headers. All these traits are built to be reusable and perhaps we will have more uses for metaprogramming in the future where this comes in handy. The intended behaviour and contract of the traits are documented in comments.
>
> RFE: https://bugs.openjdk.java.net/browse/JDK-8067790
> Webrev: http://cr.openjdk.java.net/~jwilhelm/8067790/webrev.00/
>
> In summary this allows us to get rid of the ugly macro used before and provide a better general mechanism for having generalized and specialized atomics without breaking existing ports for those platforms that do not need it. And hopefully the metaprogramming tools will come in useful in the future for perhaps similar situations.
>
> Testing:
> * All new traits have test cases using STATIC_ASSERT nailing down the intended behaviour and contract also reflected in the comments.
> * The changes passed the jprt testing ok. (Thanks to Jesper for running it!)
>
> Thanks,
> /Erik