RFR: 8186166: Generalize Atomic::cmpxchg with templates
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Thu Aug 17 17:30:39 UTC 2017
On 8/16/17 2:03 AM, 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.
>
> New webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8186166/hotspot.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8186166/hotspot.01.inc/
I like the name change to cmpxchg_using_helper.
http://cr.openjdk.java.net/~kbarrett/8186166/hotspot.01.inc/src/os_cpu/linux_aarch64/vm/atomic_linux_aarch64.hpp.udiff.html
Why does this have <byte_size> rather than the constants like the other
platforms?
http://cr.openjdk.java.net/~kbarrett/8186166/hotspot.01.inc/src/share/vm/metaprogramming/integerTypes.hpp.udiff.html
Okay, seeing this change, I have to prefer the cast using union. That's
super simple and disturbing in it's power. Thank you for the commentary
before it.
The incremental change looks good. I've reviewed this enough to be glad
when it's pushed, once all the platforms are verified.
Thanks,
Coleen
>
> 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