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

[Paul Hohensee]

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