RFR: 8186166: Generalize Atomic::cmpxchg with templates
Kim Barrett
kim.barrett at oracle.com
Wed Aug 16 06:03:02 UTC 2017
> 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/
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