RFR (M): 8184334: Generalizing Atomic with templates

[David Holmes]

Hi Kim,

On 4/08/2017 5:27 AM, Kim Barrett wrote:
>> 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

I thought NULL (aka 0 in a pointer context) was assignable to any 
pointer type without any casts. ??

> 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.

I'd like to see specific examples. I would hope that normal 
conversions/promotions would handle the majority of cases.

> (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.

My C++ is rusty. Is a derived pointer not usable where a base pointer is 
declared?? Seems fundamental to polymorphism.

The cv-qualifiers is a mess - shouldn't they really match? (I know we 
cast them on and off all over - but thats the mess part).

Regardless, this suggests to me a relaxed form might be needed for 
pointer types.

> (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.)

Enums are a disaster in C/C++ - maybe better in C++11. But I thought 
there were nice design patterns for type-safe enums that allow easy 
conversion to integer types within range?

> (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.

That's too hand-wavy for me. Many of the casts may not even be needed 
today but who is going to go through and try and figure that out! I 
don't even see that many casts in Atomic calls and the ones that are 
there seem to group into common sets in relation to pointers and intptr_t.

> 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.

Sorry I'm not convinced. Points 1,3 and 5 may suggest the need for 
relaxed pointer variants.

Also floating-point atomic ops make little sense - hardware doesn't 
provide discrete atomic ops on FP values. So the only thing you can do 
is a cas-loop on the memory location. That's why j.u.c.atomic doesn't 
support them - they'd be grossly inefficient.

Cheers,
David