RFR: 8067790: Better support for native implementations of Atomic::cmpxchg

[David Holmes]

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
>