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

[Erik Österlund]

Hi Kim,

Thank you for the review.

> On 30 dec 2014, at 23:05, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
>> On Dec 17, 2014, at 11:38 AM, Erik Österlund <erik.osterlund at lnu.se> wrote:
>> 
>> RFE: https://bugs.openjdk.java.net/browse/JDK-8067790
>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8067790/webrev.00/
> 
> This seems like a lot of infrastructure, just to eliminate a
> per-platform conditionally defined macro and the corresponding test
> for its definition. If there were more atomic operations that could be
> similarly conditionalized, with a default version that can be
> overridden by a platform-specific version, some additional
> infrastructure might be warranted. But as it stands, I don't think it
> is.
> 
> 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".

> The AtomicBase and various AtomicPlatform classes should not provide
> public cmpxchg(jbyte...) functions. They should instead provide
> protected cmpxchg_jbyte() functions, to be called by the public Atomic
> interface function (whch can make an unqualified call to
> cmpxchg_jbyte() and get the right version).
> (AtomicBase::cmpxchg_general should be renamed to cmpxchg_jbyte.)
> 
> All of the template metaprogramming infrastructure proposed by the
> offered change set can instead be accomplished by one conditionally
> defined macro, e.g.
> 
> - Along with each definition of AtomicPlatform, also define
> VM_HAS_ATOMIC_PLATFORM.
> 
> - After all the atomic_<os_arch>.hpp headers have been conditionally
> included, add the following:
> 
> #ifdef VM_HAS_ATOMIC_PLATFORM
> typedef AtomicPlatform AtomicSuper;
> #else
> typedef AtomicBase AtomicSuper;
> #endif
> 
> [An alternative to the introduction of the AtomicSuper typedef would
> be to just conditionalize the base class for Atomic.]
> 
> 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?

> 
> The rest of these comments only matter if, contrary to my suggestion
> above, the additional template metaprogramming infrastructure is
> thought necessary.
> 
> I think the name "IsBaseOfAndDerived" would be better as
> "IsBaseAndDerived".  That would also match the naming used by Boost,
> and several other similar libraries one can find via web search (some
> of which make explicit nod to the Boost name).
> 
> The current implementation of IsBaseOfAndDerived (and therefore
> IsBaseOf) may (silently) provide incorrect answers when called with
> incomplete types. C++11 is_base_of requires Derived to be complete if
> it differs from Base. Boost has the stronger requirement that both
> Base and Derived must be complete. This is an important flaw in the
> offered implementations.
> 
> The actual usage in this proposed change-set even relies on this
> flawed behavior of these metafunctions, since AtomicPlatform isn't
> always defined. This seems rather icky to me. The only way to address
> this would be to ensure that AtomicPlatform is defined for all
> targets, either via explicit empty definitions or via the
> VM_HAS_ATOMIC_PLATFORM technique described above.  But if the latter,
> then we don't need all this TMP infrastructure.
> 

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".

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?

Thanks,
Erik