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

[Erik Österlund]

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