Hi Paul,
Thank you for your reply.
Webrev for new inheritance based solution:
http://cr.openjdk.java.net/~jwilhelm/8058255/webrev.02/
In my opinion it does not create a conflict that there are two different systems. There are 3 levels in the hierarchy, all with well defined responsibilities: AtomicBase - atomics that can be generalized for all platforms, AtomicPlatform (optional) for atomics overriding AtomicBase with specialized implementations, and Atomic for atomics that always look different on all platforms (and hence has little benefit of inheritance).
With this view (and responsibilities), it is already consistent AFAIK unless we have more things that could be generalized that should be shifted up in the hierarchy which would be nice.
Do you propose we use my old solution (http://cr.openjdk.java.net/~jwilhelm/8058255/webrev.01/) for now which is less reliant on compiler support and strictly only fixes the jbyte cmpxchg in a less architecturally intrusive way, or the new cleaner inheritance variant?
Thanks,
/Erik
On 20 Oct 2014, at 16:23, Paul Hohensee <paul.hohensee at gmail.com<mailto:paul.hohensee at gmail.com>> wrote:
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<mailto: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<mailto: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<mailto: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