RFR (L, but tedious) 8188220: Remove Atomic::*_ptr() uses and overloads from hotspot

[David Holmes]

On 11/10/2017 5:45 PM, Erik Österlund wrote:
> Hi,
> 
> First off - big thanks to Coleen for this cleanup. Nice!
> 
> I think I have to take Coleen's side here regarding replace_if_null. 
> Here is why:
> 
> 1) I do not see how performing a CAS expecting NULL specifically is 
> special enough that it warrants its own operation. It does not save many 
> characters to just type it, and makes it less obvious what it does, 
> which seems unnecessary to me. Atomic ought to have the minimum atomic 
> operations required and not get cluttered with helpers.

 From the earlier review thread related to the initial templatization of 
Atomic:

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

"I've also added bool Atomic::conditional_store_ptr(T, D volatile*),
for the idiom of storing a value if the old value is NULL.  It turns
out there are about 25 occurrences of this idiom in Hotspot, so a
utility for it seems warranted.  The current implementation is just a
straightforward wrapper around cmpxchg, which means it can't take
advantage of gcc's __sync_bool_compare_and_swap.  That can be dealt
with later if desired."


> 2) To me it really does matter what each operation boils down to in 
> Atomic, especially in terms of semantics. Will my replace_if_null have 
> acquire semantics if it does not find null? Will it have trailing 
> leading, or bidirectional fencing if it succeeds, or just release 
> semantics on the store? Does it allow spurious failures? It matters to 
> me, and should preferrably not be abstracted away in my opinion.

I can buy that partially but you're stretching things given you can't 
glean those details from the name cmpxchg either.
> And if we really depend on it behaving exactly like Atomic::cmpxchg 
> semantically, I think (like Coleen) that either the name should reflect 
> that, or preferrably for me, it should be removed and replaced with an 
> explicit Atomic::cmpxchg.

I don't think we do/need-to depend on that.

> 3) I prefer not to have multiple APIs for doing the same thing. We all 
> know what happens when programmers are given the choice of two different 
> ways of expressing the same thing: they start disagreeing about how to 
> express that thing. Now in this changeset, there are inconsistencies 
> already. For example in classLoaderData.cpp:946 there is one occurence 
> of an explicit cmpxchg that expects null (for the purposes of lazy 
> initialization), while other places (e.g. nmethod.cpp:1662) use the 
> abstraction. Should that be changed now (and in subsequent changesets) 
> to use the abstraction to make the code consistent? I might think this 
> should not matter and that the explicit CAS is okay, but I can almost 
> promise somebody will have the opposite opinion. By having one way of 
> performing a CAS that expects 0, we can spend less time disagreeing 
> about which way we should CAS, and more time on other things of more 
> importance.
> 
> This is just my 50 cent, letting Coleen know she is not the only one 
> with similar thoughts.

Removing the operation is a different argument to renaming it. Most of 
the above argues for removing it. :)

Cheers,
David
-----

> I have not reviewed this completely yet - thought I'd wait with that 
> until we agree about replace_if_null, if that is okay.
> 
> Thanks,
> /Erik
> 
> On 2017-10-11 05:55, David Holmes wrote:
>> On 11/10/2017 1:43 PM, Kim Barrett wrote:
>>>> On Oct 10, 2017, at 6:01 PM, coleen.phillimore at oracle.com wrote:
>>>>
>>>> Summary: With the new template functions these are unnecessary.
>>>>
>>>> 2. renamed Atomic::replace_if_null to Atomic::cmpxchg_if_null.  I 
>>>> disliked the first name because it's not explicit from the callers 
>>>> that there's an underlying cas.  If people want to fight, I'll 
>>>> remove the function and use cmpxchg because there are only a couple 
>>>> places where this is a little nicer.
>>>
>>> I'm still looking at other parts, but I want to respond to this now.
>>>
>>> I object to this change.  I think the proposed new name is confusing,
>>> suggesting there are two different comparisons involved.
>>>
>>> I originally called it something else that I wasn't entirely happy
>>> with.  When David suggested replace_if_null I quickly adopted that as
>>> I think that name exactly describes what it does.  In particular, I
>>> think "atomic replace if" pretty clearly suggests a test-and-set /
>>> compare-and-swap type of operation.
>>
>> I totally agree. It's an Atomic operation, the implementation will 
>> involve something atomic, it doesn't matter if it is cmpxchg or 
>> something else. The name replace_if_null describes exactly what the 
>> function does - it doesn't have to describe how it does it.
>>
>> David
>> -----
>>
>>> Further, I think any name involving "cmpxchg" is problematic because
>>> the result of this operation is intentionally different from cmpxchg,
>>> in order to better support the primary use-case, which is lazy
>>> initialization.
>>>
>>> I also object to your alternative suggestion of removing the operation
>>> entirely and just using cmpxchg directly instead.  I don't recall how
>>> many occurrences there presently are, but I suspect more could easily
>>> be added; it's part of a lazy initialization pattern similar to DCLP
>>> but without the locks.
>>>
>