RFR: 8058255: Native jbyte Atomic::cmpxchg for supported x86 platforms

[Paul Hohensee]

I'd also worry about a couple of other things.  First, way back in the
mists of time, C++ compilers differed with respect to their template
implementations.  Hopefully the C++ compiles on all openjdk platforms do
what you expect.  Second, if you do this, we'll have two alternatives for
implementing Atomic, which generally isn't a good thing (vis we have two GC
frameworks).  I'd want to see every platform switch to the new way of doing
things asap and get rid of the old way.

Paul


On Mon, Oct 20, 2014 at 10:10 AM, Paul Hohensee <paul.hohensee at gmail.com>
wrote:

> Sorry for the delay in responding.
>
> Idea seems ok.  I'd like to see a webrev.
>
> You might start with your original solution (the one I reviewed) just to
> get a fix in place.
>
> Paul
>
>
> On Wed, Oct 15, 2014 at 9:47 AM, Erik Österlund <erik.osterlund at lnu.se>
> wrote:
>
>> Hi Paul,
>>
>> Thank you for the reply. I like the idea of using inheritance too.
>> I tried a new solution which is similar to the one proposed and I would
>> like to hear your comment if this is okay so we are all happy.
>>
>> The core idea is to have a general AtomicBase class which implements the
>> old unspecialized cmpxchg (and possibly other stuff to come), then allow
>> platforms to /optionally/ define their own AtomicPlatform base classes in
>> the hierarchy that derive from AtomicBase. Then the last step in the
>> hierarchy is Atomic which inherits from AtomicPlatform if it is defined and
>> otherwise AtomicBase. It picks the superclass using SFINAE. The reason for
>> this is that other ports will not be broken by this performance fix and the
>> definition of such AtomicPlatform class is completely optional. :)
>>
>> Benefits:
>> * Uses inheritance and looks nice like the inheritance based solution
>> proposed
>> * Does not break ports outside of the main repo
>> * Does not require refactoring of everything (moving all methods from
>> Atomic to AbstractAtomic for all platforms)
>> * Changes are local to only the platforms that want to add this
>> functionality
>> * Flexible for supporting the future demands
>>
>> In a bit more detail, it looks like this:
>>
>> atomic.hpp:
>>
>> class AtomicBase : AllStatic {
>>  protected:
>>   static jbyte cmpxchg_unspecialized(jbyte exchange_value, volatile
>> jbyte* dest, jbyte compare_value);
>>  public:
>>   inline static jbyte    cmpxchg    (jbyte        exchange_value,
>> volatile jbyte*        dest, jbyte        compare_value) {
>>     return cmpxchg_unspecialized(exchange_value, dest, compare_value);
>>   }
>> };
>>
>> // Linux
>> #ifdef TARGET_OS_ARCH_linux_x86
>> # include "atomic_linux_x86.hpp"
>> #endif
>>
>> // BSD
>> #ifdef TARGET_OS_ARCH_bsd_x86
>> # include "atomic_bsd_x86.hpp"
>> #endif
>>
>> // ... and the other platforms that benefit from specialized
>> AtomicPlatform ...
>>
>> class AtomicPlatform; // Forward declaration to avoid compiler error when
>> it is not defined
>>
>> // Based on some SFINAE stuff I made and will contribute if you like it.
>> AtomicSuper will be AtomicPlatform if defined in a platform specific
>> atomic_os_arch.hpp file, otherwise AtomicBase will be used
>> typedef base_class<AtomicPlatform, AtomicBase>::type AtomicSuper;
>>
>> // AtomicSuper is now the base class of Atomic, and contains the cmpxchgb
>> implementation
>> class Atomic : public AtomicSuper {
>> // ...
>>   inline static jbyte    cmpxchg    (jbyte        exchange_value,
>> volatile jbyte*        dest, jbyte        compare_value) {
>>     // Just call super class implementation
>>     return AtomicSuper::cmpxchg(exchange_value, dest, compare_value);
>>   }
>> // ...
>> };
>>
>> A new atomic_os_arch.hpp file looks like this (for platforms that support
>> this):
>> class AtomicPlatform : public AtomicBase {
>>  public:
>>   static jbyte cmpxchg(jbyte exchange_value, volatile jbyte* dest, jbyte
>> compare_value);
>> };
>>
>> And atomic_os_arch.inline.hpp contains the implementation like before,
>> but as members of AtomicPlatform instead of Atomic
>>
>> For the curious reader, the SFINAE stuff for base_class looks like this:
>>
>> template <typename Base, typename Derived>
>> struct is_base_of_host
>> {
>>   operator Base*() const;
>>   operator Derived*();
>> };
>>
>> // Statically check if two types are related in a class hierarchy
>> template <typename Base, typename Derived>
>> struct is_base_of
>> {
>>   typedef char yes[1];
>>   typedef char no[2];
>>
>>   template <typename T>
>>   static yes &check(Derived*, T);
>>   static no &check(Base*, int);
>>
>>   static const bool value = sizeof(check(is_base_of_host<Base,
>> Derived>(), int())) == sizeof(yes);
>> };
>>
>> // The ::type of this struct will be the Desired type if it was defined
>> // and a subtype of Fallback, otherwise it will be Fallback.
>> // This is useful when optionally inheriting from a specialized subtype.
>> template <class Desired, class Fallback>
>> struct base_class {
>>     template <bool, class T, class U>
>>     struct check_inheritance {
>>         typedef T type;
>>     };
>>
>>     template <class T, class U>
>>     struct check_inheritance<true, T, U> {
>>         typedef U type;
>>     };
>>
>>     typedef typename check_inheritance<is_base_of<Fallback,
>> Desired>::value, Fallback, Desired>::type type;
>> };
>>
>> If everyone is happy with this solution, then I will prepare another
>> webrev and send the patch. :)
>> Thanks for taking the time to read through this change.
>>
>> /Erik
>>
>> On 14 Oct 2014, at 19:25, Paul Hohensee <paul.hohensee at gmail.com> wrote:
>>
>> > David Holmes asked me to take a look at this
>> >
>> >>>>> There is now a new webrev with my jbyte cmpxchg changes here:
>> >>>>> http://cr.openjdk.java.net/~jwilhelm/8058255/webrev.01
>> >
>> > and comment, so...
>> >
>> > The approach is good if you're going to stay with the current framework,
>> > except that I'd get rid of VM_HAS_SPECIALIZED_BYTE_CMPXCHG (#ifdef's are
>> > the devil).  Just call cmpxchg_general (which should be named
>> > cmpxchgb_general or maybe cmpxchgb_unspecialized, btw) from an os_cpu
>> file
>> > as needed.  I.e., no change to atomic.inline.hpp needed.  Another way to
>> > look at it is, cmpxchgb_general/unspecialized is the implementation of
>> > another specialized version of cmpxchgb, on the same abstraction level
>> as
>> > the asm versions.
>> >
>> > If a rewrite is possible, I'd do it the way David suggests with an
>> > AbstractAtomic class from which per-platform Atomic classes are derived.
>> > That's the way VM_Version and ICache are defined.
>> >
>> > Paul
>>
>>
>