RFR: 8058255: Native jbyte Atomic::cmpxchg for supported x86 platforms
Paul Hohensee
paul.hohensee at gmail.com
Tue Oct 21 19:29:20 UTC 2014
Re the simple fix, if you've successfully run DaCapo using G1 on all
platforms for which you've added custom implementations, and considering
it'll be replaced (so I withdraw my previous comments and accept it as is),
consider it reviewed.
For the template fix:
A nit: in templateIdioms.hpp, put the on-one-line-by-themselves-{'s at the
end of the previous lines.
If it passes the same tests as the simple fix, it's good to go too.
Paul
On Tue, Oct 21, 2014 at 11:55 AM, Erik Österlund <erik.osterlund at lnu.se>
wrote:
> Hi Paul,
>
> I understand your defensive thinking. :) There is indeed no conflict
> between the two schemes, which was an important design goal to me.
> In this case I will need some reviews for my old solution first so I can
> push the changes.
>
> So this time, let's pretend that #define VM_HAS_SPECIALIZED_BYTE_CMPXCHG
> isn't there as it will be fixed in a cleaner way in the successive change.
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8058255
>
> Webrev for the simple fix (which I intend to push initially):
> http://cr.openjdk.java.net/~jwilhelm/8058255/webrev.03.macro/
>
> Webrev for the inheritance/template solution (which will be pushed after
> and is the intended "end" result):
> http://cr.openjdk.java.net/~jwilhelm/8058255/webrev.03.templates/
>
> Webrev for the delta from the macro solution to the inheritance/template
> solution:
> http://cr.openjdk.java.net/~jwilhelm/8058255/webrev.03.incremental/
>
> The solutions are unchanged from before except for one thing: the
> Assembler::cmpxchgb used in code stub used by awkward AMD64 windows config.
> I added a flag needed to put a REX prefix (0x40) in front of the (new)
> cmpxchgb machine code encoding when byte-sized registers are used, required
> by the hardware specification.
>
> Testing of webrev.03.macro which I want to push now:
> 1. jprt (big thanks to Jesper for helping me with this!)
> 2. I have tested that the assembly produced in CodeStub works locally by
> carefully introspecting the machine code produced compared to the machine
> code produced by inline asm.
> 3. Running through some DaCapo benchmarks (with the CodeStub cmpxchgb and
> G1 which uses jbyte cmpxchg quite a bit for card manipulation).
>
> I would like to thank Jesper again for helping me make webrevs and
> supporting me.
>
> Thanks,
> /Erik
>
> On 21 Oct 2014, at 04:33, Paul Hohensee <paul.hohensee at gmail.com> wrote:
>
> I was perhaps unclear. :) The new scheme and the old will co-exist,
> i.e., there's no conflict between the two, right? That makes transition to
> the new easier, but if we leave the old way there, then two ways of doing
> the same thing will result in more cognitive overhead and thus higher
> maintenance costs, just as is the case with the two parallel scavengers.
> So, I'd prefer the new way to supplant the old as soon as practicable.
>
> I proposed to implement your old solution as a step on the way to the
> new, so there'd be a record of how that worked. It's also the minimal
> change, so if it's in the repo and there are issues with the new way, it's
> easy to revert while a fix is done. Call it paranoia. :)
>
> Paul
>
>
> On Mon, Oct 20, 2014 at 7:04 PM, Erik Österlund <erik.osterlund at lnu.se>
> wrote:
>
>> 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> 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>
>> 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
>>>>
>>>>
>>>
>>
>>
>
>
More information about the hotspot-dev
mailing list