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

Fri Dec 19 18:51:59 UTC 2014

Hi Volker,

Thank you for reviewing this change!

> On 19 Dec 2014, at 17:30, Volker Simonis <volker.simonis at gmail.com> wrote:
> 
> Hi Erik,
> 
> nice change (did I mention already that I LOVE template metaprogramming :)

I’m glad there are more people supporting template metaprogramming for solving architectural problems! :)

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

Oh snap, that vimscript to for removing trailing whitespaces on save seems to have been disabled. :/ Fixed.

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

Sure, fixed.

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

Optimizations such as dead code elimination within a compilation unit are performed after template instantiation because before template instantiation, it’s impossible to know what the code does and hence if it is dead code or not. Therefore, I’m pretty certain that STATIC_ASSERT will always fail before finding out that the code is not needed. And cross object-file optimizations can be done at link time, but the STATIC_ASSERT will trigger way before linking. Therefore I would be very surprised if this style of testing does not work for all compilers.

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

I did by running DaCapo benchmarks using G1 (which uses this for manipulating cards), and there was no visible difference. Not surprising considering it’s all explicitly inlined as before.

> 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

Very interesting! :)
Do you want a new webrev to see that the trailing whitespaces have been removed and IsBaseOfAndDerivedTest::test was made static?

Thanks,
Erik

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