RFR: 8186166: Generalize Atomic::cmpxchg with templates

[Kim Barrett]

> On Aug 14, 2017, at 5:11 PM, coleen.phillimore at oracle.com wrote:
> 
> 
> Kim,
> 
> It's good of you to break this up.   Some initial comments:

Thanks for looking at this.

I didn't want to do all the work of dealing with all of Atomic this
way until there was general agreement that this way was acceptable.

I’m going to hold off on a new webrev for a bit, to see if there are more comments,
or problems found (esp. with the platforms I haven’t tested).

> http://cr.openjdk.java.net/~kbarrett/8186166/hotspot.00/src/os_cpu/linux_x86/vm/atomic_linux_x86.hpp.frames.html
> 
> Should all of the CmpxchgPlatform<1,4,8> functions have an assert like below?

Yes.  Done.

> Trying to find what happened to:
> 
> VM_HAS_SPECIALIZED_CMPXCHG_BYTE

VM_HAS_SPECIALIZED_CMPXCHG_BYTE no longer exists.  Platforms *must*
provide cmpxchg for bytes, either (1) directly, (2) by using
CmpxchgByteUsingInt, or (3) by some other mechanism, of which there
are presently no examples.  Requiring it be explicitly handled is
pretty simple, and makes it clear that using the helper is
intentional, and not an oversight.

> ...
> 
> http://cr.openjdk.java.net/~kbarrett/8186166/hotspot.00/src/os_cpu/solaris_sparc/vm/solaris_sparc.il.frames.html
> 
> Very nice.  After the atomics, we can file an RFE to move the rest into asm.

As David pointed out, that RFE already exists.
https://bugs.openjdk.java.net/browse/JDK-8153076

> http://cr.openjdk.java.net/~kbarrett/8186166/hotspot.00/src/os_cpu/solaris_x86/vm/atomic_solaris_x86.hpp.frames.html
> 
> I haven't worked out cmpxchg_using_stub yet.  But doing a call like this seems a lot simpler.  Why not do this for all the platforms that have stub functions rather than inline assembly?  I don't think the cost of doing a call is going to be noticeable performance wise, relative to doing synchronized operations.   Could avoid another template.

cmpxchg_using_stub exists because I got tired of typing
IntegerTypes::cast over, and over, and over...  cmpxchg_using_stub
just packages that up.

> http://cr.openjdk.java.net/~kbarrett/8186166/hotspot.00/src/share/vm/runtime/atomic.hpp.udiff.html
> 
> For the second two cmpxchg_ptr functions, can you remove the weird formatting since it's not trying to line up with anything.

Done.

> Can you add a comment just above conditional_store_ptr about what it's for? ie. initialization or rather replacing a null value.

Done.

> This comment about PlatformCmpxchg is really confusing which is D and T.   Do you mean T instead of D in the comment below, since the function () takes T ?

Yes, there were a few problems there.  I think I’ve fixed them all.

> Can you move the public above the comment so it's clear that the comment refers to this CmpxchgByteUsingInt ?  And spell out RMW.
> 
> + // Support platforms that do not provide RMW byte-level atomic access
> + // To use, derive PlatformCmpxchg<1> from this class.
> + // Can't be private: C++03 11.4/2; fixed in C++11.
> +public:
> + struct CmpxchgByteUsingInt;

I expanded RMW.  I moved the “Can’t be private…” comment to the same line as public:
I don’t really want to move the public: - it exists only as a (temporary?) workaround for a
C++98 defect, and moving it would make the intent less clear.  (The intent being that one
should squint and pretend it isn’t there.)

> I could look at IsPointerConvertible for hours and not know what it does without your previous explanation, and even now I can't recall the details.   Can you add comment hints?   Ie why is (&no)[2] declared like that, and why aren't there function declarations.   And that you're using overload resolution to try to convert From* to To*.   They don't have to be long comments because nobody's going to stare at this for a while after they get the gist of it.

I made the existing comment more explicit about what it’s accomplishing, while leaving the
mechanism described by reference to the “sizeof trick”.  I don’t really want to write a primer
on a “well known” and fundamental technique.

> nit, can you remove 'all' in this sentence.  Or "These need to be”.
> +// specializations of the class. That all needs to be provided by the

I reworded it.

>   I'm reading comments now.

:)

> So am I reading this right, this doesn't depend on integer promotions, all the integer types must be identical in sign and size?  This would be good.
> 

> +// Handle cmpxchg for integral and enum types.
> +//
> +// All the involved types must be identical.
> +template<typename T>
> +struct Atomic::CmpxchgImpl<
> + T, T, T,
> + typename EnableIf<IsIntegral<T>::value || IsRegisteredEnum<T>::value>::type>
> […]


Not just same size, same rank.  So int, long, and long long are different, even if some
pairs are the same size.  This is most likely to lead to annoying mismatches where
literal constants are used.  That doesn’t seem to come up much with cmpxchg, but
I’m expecting Atomic::add to be more “interesting”.

> Regarding platforms that don't have a cmpxchg for byte.   I see the magic now.

Magic?  It’s just class inheritance…

> Does the name of the function have to be "operator()" ?  It's very C++y and maybe standard practice but it's never called as () is it?  It could have an english verb for the name, couldn't it? Then one could do some sort of find-all pattern match on it.

The pattern you want to look for with something like this is the class name, e.g.
PlatformCmpxchg.  There’s no particularly good name for the operation that makes
it easier to find.  What would you call it?  cmpxchg?  doit?

>> - I've added the function template Atomic::condition_store_ptr, which
>> is a cmpxchg for pointer types with a NULL compare_value.  That might
>> not be the best name for it.
> 
> Maybe conditional_store_pointer (not condition).  otherwise seems ok.

“condition” was a typo; it’s presently called conditional_store_ptr.

>> - The name IntegerTypes was retained from the original RFR for
>> JDK-8184334.  However, that name no longer fits the provided
>> functionality very well, and I think a new name is needed.  Something
>> involving Bitwise, or ValueRepresentation perhaps?  I'm open to
>> suggestions.
> 
> I don't mind the IntegerTypes name even though it's not perfect. The other two names suggest other implementations of completely different things already in the vm, so I don't liek them.  There are no other sorts of IntegerTypes.

I really dislike IntegerTypes; I don’t think that’s at all evocative of what it’s become.

>> - Issues with casts/conversions provided by IntegerTypes.
>> 
> 
> So the union trick is undefined behavior and reinterpret_cast<> is also undefined behavior (sometimes?). If that's the case, I think we should pick reinterpret_cast<> then because it's intended behavior is clearer.  The union trick is really ugly.

The union trick has the benefit of being explicitly supported by some platforms, and
long use on others.  And some commentary would be needed regardless.  And it’s not
really that ugly, e.g. something like

template<typename T, typename U>
inline T convert_using_union(U x) {
  union { T t; U u; };
  u = x;
  return t;
}