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

[Erik Österlund]

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.

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.

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.

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.

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