RFR: 8067790: Better support for native implementations of Atomic::cmpxchg
Erik Österlund
erik.osterlund at lnu.se
Wed Jan 7 15:58:17 UTC 2015
Hi Coleen and David,
I was under the impression that this change was wanted to remove the error prone conditional #define for supported specialized/generalized atomics at almost any cost. I was fine with the #define myself but implemented this conditional class injection since voices were raised by reviewers to find a better way, and the actual optimization (using #define) was only conditionally accepted, given that I would provide the template inheritance solution to get rid of the #define after.
Personally I don’t think the change is too complicated, especially not from the user’s point of view as you don’t really need to understand the internals to use the traits. For this specific change, only one trait is really used by atomics, the rest are dependencies designed to be reusable “bonuses” if anyone (like myself) needs it later on (for instance for automatic closure specialization). And the trait/metafunction actually used by Atomic is SelectBaseClass which simply selects among two classes the most specific class which is defined (a general, for the AtomicBase class and optionally AtomicPlatform if implemented, otherwise ignored and AtomicBase is used as fallback).
But of course, if this fix is perceived as complicated and that this complexity is not motivated, then I understand and we can just close the issue and leave it the way it is I guess, if reviewers are now fine with that.
Thanks,
/Erik
> On 07 Jan 2015, at 05:58, David Holmes <david.holmes at oracle.com> wrote:
>
> I'm inclined to agree with Coleen - while I originally lamented the use of platform specific ifdefs and wondered about using simple inheritance, this has gone off in a direction I never envisaged. I don't think the problem being attacked is sufficiently bad to warrant the complexity** of the solution. And it still adds a bunch of platform-specific ifdefs. :(
>
> Sorry.
>
> David
> -----
>
> ** that is subjective of course - if you don't understand template meta-programming then of course it seems complex. I don't know if it remains complex if you do understand template meta-programming ?
>
> On 7/01/2015 2:27 PM, Coleen Phillimore wrote:
>>
>> Thanks David, I'm working through this thread in reverse. Thank you
>> for the context. This helps a lot.
>>
>> I am not familiar with template meta-programming either. A long time
>> ago, I was a proponent of using templates but mostly as a type-checked
>> way of avoiding duplicate code.
>>
>> In this case of template meta-programming, I don't think the
>> intellectual load of the idioms offset the ugliness of the code that
>> they are replacing. Learning these idioms is not trivial. They are
>> really hard to read. Maybe if template classes had proper names rather
>> than X, Y, and Z it would be better. In any case, using this to
>> replace code isn't a clear win in clean code. There is still the
>> duplicate declarations of AtomicPlatform. There's also the unfortunate
>> conditional inclusions in shared code:
>>
>> +// Linux
>> +#ifdef TARGET_OS_ARCH_linux_x86
>> +# include "atomic_linux_x86.hpp"
>> +#endif
>> +
>> +// Solaris
>> +#ifdef TARGET_OS_ARCH_solaris_x86
>> +# include "atomic_solaris_x86.hpp"
>> +#endif
>> +
>> +// Windows
>> +#ifdef TARGET_OS_ARCH_windows_x86
>> +# include "atomic_windows_x86.hpp"
>> +#endif
>> +
>> +// BSD
>> +#ifdef TARGET_OS_ARCH_bsd_x86
>> +# include "atomic_bsd_x86.hpp"
>> +#endif
>>
>>
>> I don't think this aids maintainability or cleanliness of the code for
>> this use case. I don't think this should be added to the code base at
>> this time.
>>
>> Thanks,
>> Coleen
>>
>> On 1/4/15, 11:34 PM, David Holmes wrote:
>>> Sorry for the top-post but just wanted to reset the context a little
>>> here. Originally [1] I made the comment:
>>>
>>> "It's fine to have generic shared approaches but there really needs to
>>> be a way to allow platform specific "overrides"."
>>>
>>> For the actual original RFR I said [2]:
>>>
>>> "Can we pause and give some more thought to a clean mechanism for
>>> allowing a shared implementation if desired with the ability to
>>> override if desired. I really do not like to see CPU specific ifdefs
>>> being added to shared code. (And I would also not like to see all
>>> platforms being forced to reimplement this natively).
>>>
>>> I'm not saying we will find a simple solution, but it would be nice if
>>> we could get a few folk to think about it before proceeding with the
>>> ifdefs :)"
>>>
>>> Erik then proposed three alternative approaches [3] and the simple
>>> variant was chosen [4] and presented for review. However Paul Hohensee
>>> also made comments about an inheritance-based approach and Erik
>>> floated the first template-based variant [5] and there was some
>>> discussion with Paul and Kim. But we then ended up with the simple
>>> solution, leaving an inheritance-based solution for future work [6]
>>> (which is what is what is being discussed now).
>>>
>>> This template-based meta-programming is not something I have any
>>> familiarity with. My initial thought is this seems overkill for the
>>> situation we have with the Atomic class - but as I said I'm ignorant
>>> of the technique being used here.
>>>
>>> David
>>> -----
>>>
>>> [1]
>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-September/015292.html
>>>
>>> [2]
>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-September/015303.html
>>>
>>> [3]
>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-September/015362.html
>>>
>>> [4]
>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-September/015401.html
>>>
>>> [5]
>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-October/015586.html
>>>
>>> [6]
>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-November/015889.html
>>>
>>>
>>> On 2/01/2015 12:07 AM, Erik Österlund wrote:
>>>> Hi Kim,
>>>>
>>>>> On 01 Jan 2015, at 07:42, Kim Barrett <kim.barrett at oracle.com> wrote:
>>>>>
>>>>> On Dec 31, 2014, at 6:12 AM, Erik Österlund <erik.osterlund at lnu.se>
>>>>> wrote:
>>>>>>
>>>>>>> The rest of these comments only matter if, contrary to my suggestion
>>>>>>> above, some additional infrastructure is thought necessary.
>>>>>>>
>>>>>>
>>>>>> This is what I originally thought too and hence the current
>>>>>> solution. But voices said that using macros for this is, I quote,
>>>>>> "that bad".
>>>>>>
>>>>>>> […]
>>>>>>>
>>>>>>> This scales with the number of AtomicPlatform definitions, rather
>>>>>>> than
>>>>>>> the number of specialized functions as happens with the existing
>>>>>>> code.
>>>>>>
>>>>>> So your proposal is to still use ugly macros but move it to the
>>>>>> class level instead, even though at the moment there is really only
>>>>>> one member function that needs it. That feels a bit meh to be
>>>>>> honest. IMO if we want to do this right, why go half the way and
>>>>>> still rely on ugly macros?
>>>>>
>>>>> Please lay off with the perjorative "ugly macros". The preprocessor
>>>>> is part of C++, and isn't going away. It's a tool that can be used,
>>>>> or abused, and I think this is a fine use. To me, "ugly" is adding
>>>>> 800+ lines of code / tests / comments to eliminate a dozen lines of
>>>>> flag macro definitions and associated #ifdef’s.
>>>>
>>>> Well people are entitled to have different opinions of course. As I
>>>> already mentioned, I did this because other reviewers (as well as me)
>>>> found it ugly to use macros for conditionally specializing, which is
>>>> the whole motivation of doing this, and was well understood from the
>>>> beginning. But I’m fine with leaving it as macros if this is in
>>>> general preferred and opinions have suddenly changed in this matter.
>>>>
>>>> Personally I don’t see how the number of rows of code define
>>>> uglyness. Reusability, testing, documentation and well definedness in
>>>> contracts/behaviours all lead to more lines of code, but I don’t
>>>> think that equals uglyness. So while I disagree more code is uglier
>>>> because it’s more, it’s all up to opinion and it’s quite pointless to
>>>> discuss; you are entitled to think that is ugly.
>>>>
>>>>> If that additional infrastructure were amortized across more use-cases
>>>>> then it might be more appealing. I might even have some additional
>>>>> use-cases for something along that line, though not in its present
>>>>> form. (More on that below.)
>>>>
>>>> Yeah I specifically had in mind the wish to make a split between
>>>> commercial extensions to open source code actually. This pattern
>>>> allows easy optional class inheritance injection as a general design
>>>> pattern to achieve this. And for my own purposes the ability to
>>>> define asymmetric dekker synchronization to elide storeload fences
>>>> when the platform (e.g. TSO) supports it but otherwise revert to a
>>>> generalized implementation (using storeload fences). This is why I
>>>> think a clean reusable strategy is almost always better because it is
>>>> widely applicable, even if there would not be obvious other examples
>>>> where it is useful.
>>>>
>>>>>
>>>>>>> I think the name "IsBaseOfAndDerived" would be better as
>>>>>>> "IsBaseAndDerived”. […]
>>>>>>>
>>>>>>> The current implementation of IsBaseOfAndDerived (and therefore
>>>>>>> IsBaseOf) may (silently) provide incorrect answers when called with
>>>>>>> incomplete types. […]
>>>>>>>
>>>>>>> The actual usage in this proposed change-set even relies on this
>>>>>>> flawed behavior of these metafunctions, since AtomicPlatform isn't
>>>>>>> always defined. […]
>>>>>>>
>>>>>>
>>>>>> So what you are saying here is that I should rename
>>>>>> IsBaseOfAndDerived to IsBaseAndDerived to more closely resemble
>>>>>> other external libraries we do not use. But if I do that my
>>>>>> behaviour is "incorrect" because it is not identical to that of the
>>>>>> external library. And therefore my change would not work if it was
>>>>>> made "correctly”.
>>>>>
>>>>> The proposed IsBaseOfAndDerived and IsBaseOf are being described and
>>>>> placed as general utilities. Their names are strongly reminiscent of
>>>>> existing facilities in other libraries, including the (C++11) standard
>>>>> library. Conforming to these external standards will avoid surprises,
>>>>> especially when the proposed facilities are even described as being
>>>>> the same (except for a noted corner case that is both hard to deal
>>>>> with in a portable manner and not actually very interesting, so I'm
>>>>> not complaining about that) in the review request email.
>>>>>
>>>>> Additionally, I think quietly providing a potentially incorrect answer
>>>>> in the case of incomplete types renders the offered code quite broken
>>>>> for use as a general utility. I think the requirement for complete
>>>>> types by the well-known / standard facilities is well justified.
>>>>>
>>>>
>>>> While I disagree it is universally incorrect (as long as it is well
>>>> defined and tested) to not have the exact same requirement on
>>>> completeness on the parameters as C++11/boost, who also already have
>>>> different behaviour in this exact regard as pointed out by yourself,
>>>> I can try to fix it if it bugs you, but it will make the
>>>> infrastructure even larger which seems to be a concern.
>>>>
>>>>>> On the contrary this is a well tested feature on all our supported
>>>>>> compilers and I did not intend to engineer it to be identical to
>>>>>> some external library if it only causes trouble rather than
>>>>>> helping. This is our own class and it can do what we want it to do
>>>>>> and be called what we want it to be called, and I think that is
>>>>>> fine as long as it is well tested, which it is. At least this is my
>>>>>> opinion. Anyone else agree?
>>>>>
>>>>> Problem is, I think it's well tested code to solve the wrong problem.
>>>>>
>>>>> I suggest that what we're really looking for is not a base/derived
>>>>> relationship between a pair of classes, but that we actually want to
>>>>> determine whether a platform-specific class exists.
>>>>>
>>>>> As a result, I think there is more infrastructure in this change set
>>>>> than is actually needed. The present implementation of
>>>>> SelectBaseClass uses IsBaseOf, but doesn't need a lot of the
>>>>> functionality of that metafunction. There is no actual need for a
>>>>> Base/Derived relationship between the (possibly not defined)
>>>>> platform-specific class and the default platform-generic class, so the
>>>>> use of IsBaseOf is an unnecessary restriction that only
>>>>> serendipitously tests for defined or not, due to the previously
>>>>> discussed defect in this particular implementation. I also think the
>>>>> name SelectBaseClass is perhaps overly general.
>>>>
>>>> The main idea of SelectBaseClass is to inject an optional class into
>>>> the class hierarchy for specialization of generalized behaviour. It
>>>> is absolutely critical (and intended) that this class is constrained
>>>> to be a derived class of the fall-back class - it is not an artifact
>>>> nor an accidental behaviour. In the hierarchy A is-maybe-a B is-a C,
>>>> A must be able to rely on the superclass being at least C-like so
>>>> that the behaviour of the superclass can be used reliably and
>>>> transparently regardless of the existence of the optional B. If this
>>>> contract would not hold, I think it could lead to some really weird
>>>> and unreliable code which should not be encouraged (at least not
>>>> according to the Liskov substitution principle etc).
>>>>
>>>> So what I need in the contract is a check if the optional class is
>>>> defined and if it fits in the inheritance hierarchy: they are both
>>>> important IMO.
>>>>
>>>> Yes it could have been less code. From the beginning I had a single
>>>> trait class that checked for both the inheritance and whether the
>>>> class is defined or not but you said you wanted the behaviour to more
>>>> closely match that of third party libraries. This is why there is now
>>>> a lot more code to more closely (but still not exactly) match that
>>>> behaviour.
>>>>
>>>>> I might be able to use a similar approach in some code I've been
>>>>> working on as a background task. And while writing this reply I've
>>>>> thought of another place that might benefit from something along those
>>>>> lines. Thinking about the code under review in that context, I think
>>>>> some changes would make these other possible use-cases easier and
>>>>> clearer.
>>>>>
>>>>> I believe what is actually needed is a metatfunction something like:
>>>>>
>>>>> /**
>>>>> * Metafunction whose nested type member is Default if Specific is
>>>>> * incomplete, otherwise Specific.
>>>>> *
>>>>> * Specific and Default must both be non-cv qualified class types, and
>>>>> * must not be the same type.
>>>>> *
>>>>> * If Specific is incomplete at the point where this metafunction is
>>>>> * invoked, it must never be defined elsewhere in the program.
>>>>> */
>>>>> template<typename Specific, typename Default>
>>>>> struct SelectPlatformBase;
>>>>>
>>>>> [Note: I'm not wedded to that name.]
>>>>>
>>>>> Now, it turns out to be hard / impossible to write a generic
>>>>> is_complete<T> metafunction, due to ODR. Whether a type is complete
>>>>> can vary between translation units, and even within a translation
>>>>> unit. Having is_complete<T>::value have different values depending on
>>>>> where it is instantiated is an ODR violation. (One can make progress
>>>>> by using anonymous namespaces and macro wrappers with __LINE__
>>>>> concatenation, but there is still the fundamental question of whether
>>>>> is_complete<T> really even makes semantic sense.)
>>>>
>>>> Yes and that makes me wonder if we care enough about the completeness
>>>> semantics being the same as third party libraries (who are already
>>>> inconsistent) if we do not need them to be, and on the contrary
>>>> benefit from them not to be.
>>>>
>>>>> However, we don't need a fully general is_complete utility. We have a
>>>>> much more restricted use-case, where we want to select an optionally
>>>>> defined platform-specific class if it exists, and otherwise fall back
>>>>> to a more generic class. And we're not using this in arbitrary
>>>>> contexts, but only to select a platform-specialized implementation if
>>>>> such exists.
>>>>>
>>>>> Here's a lightly tested implementation of SelectPlatformBase:
>>>>>
>>>>> // --- SelectPlatformBase
>>>>>
>>>>> template<bool b, typename T = void>
>>>>> struct EnableIf { typedef T type; };
>>>>>
>>>>> template<typename T>
>>>>> struct EnableIf<false, T> { };
>>>>>
>>>>> template<bool Cond, typename Then, typename Else>
>>>>> struct If { typedef Then type; };
>>>>>
>>>>> template<typename Then, typename Else>
>>>>> struct If<false, Then, Else> { typedef Else type; };
>>>>>
>>>>> // Metafunction whose nested value member is true if T is defined
>>>>> // (complete), and false if T is incomplete. T must be a non-cv
>>>>> // qualified class type. If T is incomplete at the point where this
>>>>> // metafunction is invoked, it must never be defined elsewhere in the
>>>>> // program.
>>>>> template<typename T>
>>>>> class IsClassDefined {
>>>>> typedef char yes[1];
>>>>> typedef char no[2];
>>>>>
>>>>> template<typename U>
>>>>> static typename EnableIf<sizeof(U) == sizeof(U), yes>::type &
>>>>> check(U*);
>>>>> static no& check(...);
>>>>>
>>>>> public:
>>>>> static const bool value = sizeof(check((T*)0)) == sizeof(yes);
>>>>> };
>>>>>
>>>>> template<typename Specific, typename Default>
>>>>> struct SelectPlatformBase {
>>>>> typedef typename If<
>>>>> IsClassDefined<Specific>::value, Specific, Default>::type type;
>>>>> };
>>>>>
>>>>> // --- end SelectPlatformBase
>>>>>
>>>>> That's ~30 lines of code (+ tests and comments TBD) to do precisely
>>>>> what we need, replacing ~100 lines (+ tests and comments) that have
>>>>> various issues. (If and EnableIf should probably be hoisted out as
>>>>> separate utilities.) We don't actually need SelectPlatformBase, and
>>>>> could instead just directly use the If and IsClassDefined
>>>>> metafunctions, but giving this functionality a name might be clearer.
>>>>
>>>> As stated, I think it’s dangerous to allow conditional inheritance
>>>> where there is no inheritance constraint on the optional class.
>>>> Therefore I do not think this is better, although I agree it is smaller.
>>>>
>>>> I agree that our use case here is smaller. If the amount of code is
>>>> the problem and less code (and hence fewer reusable components) is
>>>> preferred, then the original implementation with a simple
>>>> IsBaseOfAndDerived does the trick (without checking types/cv
>>>> qualifiers) and could be done in just a few lines, and this is what I
>>>> originally did before you told me to expand it to more closely
>>>> resemble external libraries:
>>>>
>>>> template<typename Base, typename Derived>
>>>> class IsBaseOfAndDerived {
>>>> typedef char yes[1];
>>>> typedef char no[2];
>>>>
>>>> template<typename T>
>>>> static yes &check(Derived*, T);
>>>> static no &check(Base*, int);
>>>>
>>>> template<typename B, typename D>
>>>> struct IsBaseOfHost {
>>>> operator B*() const;
>>>> operator D*();
>>>> };
>>>> public:
>>>> static const bool value = sizeof(check(IsBaseOfHost<Base,
>>>> Derived>(), int())) == sizeof(yes);
>>>> };
>>>>
>>>> It checks the inheritance and existence of the Derived class which is
>>>> exactly the constraints I need.
>>>>
>>>> If you do not want to expose it publicly and remove the reusability
>>>> because it does not have identical semantics as third party libraries
>>>> regarding the requirements of the Derived type to be complete, it
>>>> could be an internal class of SelectBaseClass.
>>>>
>>>> I’m not going to value the reusability vs LoC, I leave that decision
>>>> to you.
>>>>
>>>>> However, while I think there are other use cases for this specific
>>>>> utility, I still think the proposed change set as a whole is adding a
>>>>> lot of code just to avoid a a few lines of macro usage. That seems
>>>>> like a poor tradeoff to me.
>>>>
>>>>
>>>> Okay.
>>>>
>>>> In the end, we have to make a choice - what is more important, that
>>>> the traits resemble some external library or code size. Because last
>>>> time I proposed a small change and you wanted it to be closer to
>>>> C++11 behaviour and I made it closer to that at the expense of more
>>>> code. Now you are saying I have to make it even tighter to C++11 (or
>>>> boost?), but also don’t want more lines of code which I believe is
>>>> impossible given that I want to constrain both the class hierarchy to
>>>> be reliable and check the existence of the optional class. This
>>>> brings me to a few architectural questions which I may have (strong)
>>>> opinions about but are not up to me to decide.
>>>>
>>>> 1. Do we want to replace the macros with template metaprogramming? 2
>>>> reviewers (and me) liked the metaprogramming approach and it was
>>>> implemented because they (like me) did not want the macros. But you
>>>> have a different opinion. I can’t judge who is right and wrong here.
>>>>
>>>> 2. Do we want to minimize the code size at the cost of reusability by
>>>> making contracts of dependent components weaker and putting them as
>>>> private classes in the SelectBaseClass?
>>>>
>>>> 3. If we do want template metaprogramming and want reusable
>>>> components, the question is, is it important that the specification
>>>> of completeness (which in general is almost impossible to implement
>>>> correctly in a portable way as you pointed out) of the template
>>>> arguments in IsBaseOf is identical as C++11/boost even though no code
>>>> needs it to be and on the contrary some code benefits from it not to
>>>> be? In that case which one of them do we, because they are already
>>>> different anyway? There is a tradeoff between code size and
>>>> complexity vs copying either one of the external libraries regarding
>>>> requirements of completeness of template parameters.
>>>>
>>>> Thanks,
>>>> /Erik
>>>>
>>
More information about the hotspot-dev
mailing list