RFR: 8186166: Generalize Atomic::cmpxchg with templates

David Holmes david.holmes at oracle.com
Wed Aug 16 23:22:25 UTC 2017

Hi Kim,

On 16/08/2017 4:03 PM, Kim Barrett wrote:
>> On Aug 15, 2017, at 2:57 AM, David Holmes <david.holmes at oracle.com> wrote:
>> Hi Kim,
>> All points in your response below noted - in particular the problem with enum definition locations for registration.
>> Again many thanks for your perseverance on this and for making things more palatable for those of us not familiar with this style of programming.
>> Here's the rest of the review - with some things already addressed, or flagged, by Coleen's review and your responses ("sizeof trick"!!). Many of my initial questions/comments have resolved themselves as I've gotten further through the review. :) Just a few queries and a couple of suggestions left.
> Thanks for looking at this, and for learning a lot about some parts
> of C++ in a very short time.

I think I still have only learnt a little :)

> New webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8186166/hotspot.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8186166/hotspot.01.inc/

Overall this seems okay to me and I have no further major comments or 
discussion comments.


> This includes changes mentioned in reply to Coleen's review.  It also
> contains changes discussed below in response to David's review.
> David suggested renaming IntegerTypes to PrimitiveTypes.  That isn't
> entirely satisfying to me.  This class is, to me, more about
> conversion than types per se.  I'm presently leaning toward
> PrimitiveConversions, especially since, as discussed in a previous
> message, I think floats should remain.  I haven't made any change to
> the name yet though.  I'll probably leave the name change to the very
> end, before pushing, to ease incremental reviews, and to allow more
> time for bike shedding.
> I've changed over to using the union trick.  This resulted in fewer
> cases to handle, so allowed further simplification of IntegerTypes.
> David suggested renaming conditional_store_ptr to replace_if_null.
> I've made that change.
> I've simplified the conversion of the compare_value in the pointer
> case of cmpxchg.  I realized there's no need for any
> metaprogramming-based type manipulation; just a straight const_cast to
> D* does what's needed.
> I changed the type of BitMap::_pop_count_table to add a const
> qualifier.  It should have always been there, and provides an example
> of why requiring exact type matching in the pointer case is harmful.
> Additional comments inline below.
>> src/os_cpu/aix_ppc/vm/atomic_aix_ppc.hpp
>> The actual body has:
>> 435   long old_value;
>> rather than "T old_value". I presume the AIX compiler can't handle using T with inline asm?
> There wasn't any good reason not to do that, that I know of, so I've
> made that change.  Similarly for linux_ppc.  I made a similar change
> to linux_s390 for the 8-byte case, but not 4-byte, where the existing
> code is declaring old to be unsigned long rather than the expected
> unsigned int.  I don't know if that's a bug or something about the
> s390.  Looking at the s390 code, I think there might be missing
> compile barriers that allow (non-volatile) memory accesses to be
> reordered across the operation.  I'll bring those issues up directly
> with the SAP folks.
>> src/os_cpu/windows_x86/vm/atomic_windows_x86.hpp
>> We must revisit why we are using the stub generator on Windows!
> Not my problem :)
>> src/share/vm/gc/shared/workgroup.cpp
>>     if (old == 0) {
>> !     old = Atomic::cmpxchg(1u, &_tasks[t], 0u);
>>     }
>>     assert(_tasks[t] == 1, "What else?");
>> I don't understand why we can compare against 0 and 1, yet must pass 0u and 1u to our extremely clever template function. :(
> The usual conversions are applied to the comparison.  Comparing a
> signed with an unsigned will generate a warning with some options for
> some compilers, but not when the value that needs to be converted is a
> constant (or maybe only a literal) known to be safe to convert.  In
> the cmpxchg, we're requiring an exact match, and no conversions are
> applied.  Remember that the deduced template parameters take on the
> actual (and exact) static types of the arguments.  We could manually
> apply conversions in the template implementation, possibly attempting
> to only apply safe conversions.  That would require removing the "all
> same types" requirement for the integral/enum case, and some effort
> for a case that can be handled by just using the correct types.
>> src/share/vm/oops/oop.inline.hpp
>> 400                                          volatile HeapWord *dest,
>> 411     narrowOop old = Atomic::cmpxchg(val, (narrowOop*)dest, cmp);
>> 418     return Atomic::cmpxchg(exchange_value, (oop*)dest, compare_value);
>> do the casts here indicate we should have some conversion function from HeapWord to oop and narrowoop?
> Maybe, but not my problem :)  I was expecting there to be lots of
> narrowOop* casts, but there are only about 25.  Though there are
> probably a lot more where the type is a template parameter, so not as
> obvious.
>> src/share/vm/runtime/atomic.hpp
>> So the overall chain of things is:
>> cmpxchg -> CmpxchgImpl -> PlatformCmpxchg<N> ->  inline assembly; or
>>                                             |-> cmpxchg_using_stub -> external .s/.il definition.
>> I'm unclear why we need CmpxchgImpl - or more accurately I'm unclear why all the type "dispatching" that CmpxchgImpl does can't be done by the top-level Atomic::cmpxchg template?
> We can't have just one cmpxchg template that handles the various
> cases.  The case handling code may not even compile without the
> correct static context.  For example, we can't have
>    if (IntegerTypes::Translate<T>::value) {
>      return IntegerTypes::Translate<T>::recover(...);
>    }
> because if T doesn't have a translator, there is no recover function
> to reference.  So we need separate compilation contexts for the
> different cases.
> [C++17 adds "if constexpr (...)", where only the selected clause is
> subjected to some compiler stages.  Using that, the above example
> *would* work.  I don't think this change should wait for that...]
> One way to get those separate contexts is through the use of a helper
> class like CmpxchgImpl, and using SFINAE to guide the selection.
> Another is to use overloading and use SFINAE to prune the applicable
> set.  Both generally work, and I think it's mostly a matter of style
> and personal preference which to use.  Erik seems to prefer
> overloading, and avoiding the extra helper class.  I think the syntax
> for function SFINAE is pretty horrible, and usually prefer the extra
> helper class, though that shifted when I was using C++11.  In C++11
> one can write a (clever :) macro we'll call ENABLE_IF, such that one
> can write (for example)
> template<typename T, ENABLE_IF(IntegerTypes::Translate<T>::value)>
> inline T Atomic::cmpxchg(T exchange_value,
>                           T volatile* dest,
>                           T compare_value,
>                           cmpxchg_memory_order order) {
>    return IntegerTypes::recover(...);
> }
> instead of a C++98 form like
> template<typename T>
> inline typename EnableIf<IntegerTypes::Translate<T>::value, T>::type
> Atomic::cmpxchg(T exchange_value,
>                  T volatile* dest,
>                  T compare_value,
>                  cmpxchg_memory_order order) {
>    return IntegerTypes::recover(...);
> }
>> So cmpxchg_using_stub exists to deal with any needed conversions (type or order) between the PlatformCmpxchg API and the actual platform-specific functions (if they exist) - ok. Only nit I have here is the use of "stub" because when I see that I think of the stubGenerator, and this has nothing to do with that. How about cmpxchg_adapter? (And perhaps StubFn should be something else ... I don't see it as a stub, as its the real function to do the work ... could just be Fn?
> I first recognized something like this would be useful when looking at
> the windows port, which *is* using the stub generator, and got "stub"
> stuck in my head.  cmpxchg_adapter seems like a fine name for this,
> except for spelling confusion over "adapt[eo]r".  I personally tend to
> use the (apparently much less frequent) "adaptor", which is also the
> spelling used by the C++ standard and Boost.  I read somewhere that
> Java usage prefers "adapter".
> In the description comment I call it a "helper function".  I agree
> "stub" has wrong implications, so going with cmpxchg_using_helper.
>> inline T Atomic::PlatformCmpxchg<1>::operator()
>> Coleen flagged that use of operator() is somewhat obscure, and I tend to agree. At the moment calling it cmpxchg might seem redundant, but what if the naming scheme were eg:
>> Atomic::Platform<1>:cmpxchg
>> Atomic::Platform<1>:add
>> etc?
> Then, for example, we can't implement cmpxchg for the 1-byte case by
> deriving Atomic::Platform<1> from CmpxchgByteUsingInt.  I think there
> will be opportunities for similar utility classes for other
> operations; I already have one in mind for (part of) Atomic::add,
> which would apply to all sizes, not just the 1-byte case.
> Platform< ?? >::cmpxchg is also a bit harder to search for than
> PlatformCmpxchg.
> I think function objects are the normal and natural way to write this
> sort of thing.  I realize its not common style for Hotspot.  But then,
> neither is metaprogramming in general.
>> 182   template<typename StubType, typename StubFn, typename T>
>> 183   static T cmpxchg_using_stub(StubFn stub_fn,
>> 184                               T exchange_value,
>> 185                               T volatile* dest,
>> 186                               T compare_value);
>> Templates 101 question: why when we instantiate this template do we only need to pass StubType explicitly eg:
>> cmpxchg_using_stub<jlong>(_Atomic_cmpxchg_long, exchange_value, dest, compare_value);
>> are the other types inferred from the function arguments?
> Yes.  Those unsupplied template parameters are "deduced" (to use the
> proper jargon).  Note that type parameter deduction is only provided
> for function templates, not class templates.  (Class templates get
> deduction in C++17.)
>> IsPointerConvertible ... yep now I get it. I got the sizeof trick quickly but was missing the actual conversion check - now realizing that test(<value of type From*>) will select test(To*) if there is a conversion, and test(...) if not. :)
> By George, he's got it!
>> That's it. :)
>> Thanks,
>> David
> Thanks for your perseverance.

More information about the hotspot-dev mailing list