RFR (L, but tedious) 8188220: Remove Atomic::*_ptr() uses and overloads from hotspot
erik.osterlund at oracle.com
Wed Oct 11 07:45:59 UTC 2017
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
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
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.
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.
>> 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
>> 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.
More information about the hotspot-dev