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.
Thanks,
David
-----
> 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