RFR: 8058255: Native jbyte Atomic::cmpxchg for supported x86 platforms
Paul Hohensee
paul.hohensee at gmail.com
Wed Oct 22 03:08:59 UTC 2014
Can't complain as long as you've tested on all available platforms. :)
No need for a new webrev for formatting changes. You're good to go afaic.
Paul
On Tue, Oct 21, 2014 at 8:38 PM, Erik Österlund <erik.osterlund at lnu.se>
wrote:
> Hi Paul,
>
> On 21 Oct 2014, at 21:29, Paul Hohensee <paul.hohensee at gmail.com> wrote:
>
> 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.
>
>
> Just to clarify, I don't have access to all platforms unfortunately, so
> the solaris (32 and 64 bit) and windows 32-bit approaches have still only
> been tested with jprt and careful manual reviewing, so an extra eye should
> probably glance over it. The windows AMD64 variant with runtime-produced
> cmpxchgb, linux and bsd variants have been tested with DaCapo + G1. Sorry
> if I was not clear enough with this.
>
> For the template fix:
>
> A nit: in templateIdioms.hpp, put the on-one-line-by-themselves-{'s at
> the end of the previous lines.
>
>
> The styling of templateIdioms.hpp has been fixed now. (Do you need a new
> webrev for this?)
>
> If it passes the same tests as the simple fix, it's good to go too.
>
>
> The inheritance/template solution went through the same tests as the
> first and it's fine.
> Big thanks again to Jesper for running it through jprt.
>
> Big thanks for reviewing my changes! :)
>
> /Erik
>
>
> 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