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

[David Holmes]

Hi Coleen,

Thanks for doing this tedious cleanup!

It was good to see so many casts disappear; and sad to see so many have 
to now appear in the sync code. :(

There were a few things that struck me ...

Atomic::xchg_ptr turned into Atomic::xchg; yet for the stub generator 
routines atomic_xchg_ptr became atomic_xchg_long - but I can't see where 
that stub will now come into play?

---

src/hotspot/share/gc/shared/taskqueue.inline.hpp

+  return (size_t) Atomic::cmpxchg((intptr_t)new_age._data,
+                                  (volatile intptr_t *)&_data,
+                                  (intptr_t)old_age._data);

The actual types here should be size_t, can we now change it to use the 
real type?

---

src/hotspot/share/oops/cpCache.cpp

  114 bool ConstantPoolCacheEntry::init_flags_atomic(intptr_t flags) {
  115   intptr_t result = Atomic::cmpxchg(flags, &_flags, (intptr_t)0);
  116   return (result == 0);
  117 }

_flags is actually intx, yet above we treat it as intptr_t. But then later:

  156   if (_flags == 0) {
  157     intx newflags = (value & parameter_size_mask);
  158     Atomic::cmpxchg(newflags, &_flags, (intx)0);
  159   }

its intx again. This looks really odd to me.

---

src/hotspot/share/runtime/objectMonitor.inline.hpp

The addition of header_addr() made me a little nervous :) Can we add a 
sanity assert either inside it (or in synchronizer.cpp), to verify that 
this == &_header  (or monitor == monitor->header_addr())

---

src/hotspot/share/runtime/synchronizer.cpp

  // global list of blocks of monitors
-// gBlockList is really PaddedEnd<ObjectMonitor> *, but we don't
-// want to expose the PaddedEnd template more than necessary.
-ObjectMonitor * volatile ObjectSynchronizer::gBlockList = NULL;
+PaddedEnd<ObjectMonitor> * volatile ObjectSynchronizer::gBlockList = NULL;

Did this have to change? I'm not sure why we didn't want to expose 
PaddedEnd, but it is now being exposed.

Thanks,
David
-----


On 11/10/2017 11:50 PM, coleen.phillimore at oracle.com wrote:
> 
> Please review version .02 which removes use of replace_if_null, but not 
> the function.  A separate RFE can be filed to discuss that.
> 
> open webrev at http://cr.openjdk.java.net/~coleenp/8188220.02/webrev
> 
> Thanks,
> Coleen
> 
> On 10/11/17 7:07 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 10/11/17 4:12 AM, Robbin Ehn wrote:
>>> On 10/11/2017 10:09 AM, David Holmes wrote:
>>>> On 11/10/2017 5:45 PM, Erik Österlund wrote:
>>>>
>>>> Removing the operation is a different argument to renaming it. Most 
>>>> of the above argues for removing it. :)
>>>
>>> +1 on removing
>>
>> Thank you for all your feedback.  Erik best described what I was 
>> thinking.  I will remove it then.  There were not that many instances 
>> and one instance that people thought would be useful, needed the old 
>> return value.
>>
>> Coleen
>>>
>>> Thanks, Robbin
>>>
>>>>
>>>> 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.
>>>>>>>
>>>>>
>>
>