RFR (M): 8184334: Generalizing Atomic with templates

[Kim Barrett]

> On Aug 1, 2017, at 11:35 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Kim,
> 
> Good planning on Erik's part to go on vacation just as I have returned ;-)
> 
> On 1/08/2017 4:18 AM, Kim Barrett wrote:
>>> On Jul 28, 2017, at 12:25 PM, Erik Osterlund <erik.osterlund at oracle.com> wrote:
>>> 
>>> Hi Andrew,
>>> 
>>> In that case, feel free to propose a revised solution while I am gone.
>> Erik has asked me to try to make progress on this while he's on
>> vacation, rather than possibly letting it sit until he gets back.
> 
> Okay, while Erik is gone perhaps you can clarify a few things. As Andrew and Roman have expressed, I too find this:
> 
> +   template <typename T, typename U, typename V>
> +   inline static U cmpxchg(T exchange_value, volatile U* dest, V compare_value, cmpxchg_memory_order order);
> 
> totally unintuitive and unappealing. I do not understand the rationale for this this at all. It does not make any sense to me to allow T, U and V to be different types (even if constrained). It has been stated that if we force them to all be the same there is some problem with literals and the need for casts, but I don't understand what that problem would be.

This response is about the types for a cmpxchg template.  I'll respond
to other comments separately.

It is certainly simpler to implement if the types for all three
arguments are required to be the same. 

I suggested Erik do that, but he reported running into "lots" of
compilation failures.  I guess Erik was less persistent than Andrew in
working through them.  Given Andrew's reported small number, I've also
done that experiment and looked at the failing cases.  I think
(nearly?) all would be improved by making the argument types match.
There's also one similar case for xchg.

However, there are use-cases that I think are reasonable which don't
immediately fit that restriction.

(1) cmpxchg(v, p, NULL), to store a pointer if no pointer is already
present.  This can be used as an alternative to DCLP. One way to deal
with this might be an overload on std::nullptr_t and use nullptr, but
that requires C++11.  We don't have any current uses of this that I
could find, but it's a sufficiently interesting idiom that I'm
relucant to forbid it.  But such idiomatic usage could be wrapped up
in its own little package that can deal with the restriction.

(2) The use of literals can make getting a type match more difficult,
especially when the pointee type doesn't have portable syntax (like
intx and uintx).  But using properly typed named values solves this,
and may be seen as an improvement over magic literal values.

(3) Passing a derived pointer as the new value when updating an atomic
pointer seems reasonable to me.  A derived pointer compare value seems
somewhat less so.  Similarly, new and compare values that have pointee
types that are less cv-qualified than the destination also seems
reasonable.

(4) Several of the problematic cases involve enums, which aren't
especially well handled by the original proposal. From Erik's
explorations I knew there were some issues with enums. Having now
looked at some of these cmpxchg problems, I think we should try harder
to deal well with enums. One problem is that C++11 std::is_enum is at
least hard (maybe impossible?) to portabily emulate in C++98, so some
other mechanism would be needed to recognize them. Probably the
simplest is a registration mechanism for enum types that are used for
atomic-access values (e.g a type predicate defaulting to false, and
specializations for relevant enum types, near their definitions.)

(5) One of our goals is to eliminate, as much as possible, the need
for explicit casts and conversions in uses of this API.  We think the
existing widespread use of casts makes code difficult to understand
and is a source of bugs.  I think the small number of call sites
affected by requiring the cmpxchg types all be the same is in no small
measure a result of that widespread use of casts in calling code.
Unfortunately, Hotspot is rife with boundaries across which there are
type mismatches (and often inconsistencies even within a single
logical chunk of code).  We don't think that's a good thing, but it's
the context in which we were developing this change.

I think the proposed change may be more lax than it should / could
be, but I think there are valid reasons to not require the types for
cmpxchg arguments to all match.