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

Thu Oct 12 11:52:43 UTC 2017

On 10/12/17 3:23 AM, David Holmes wrote:
> 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. :(

The sync code has _owner field as void* because it can be several 
things.  I didn't try to
>
> 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?

http://cr.openjdk.java.net/~coleenp/8188220.02/webrev/src/hotspot/cpu/x86/stubGenerator_x86_64.cpp.udiff.html

I tried to remove it but windows x64 uses a stub for xchg (and others).  
There was a preexisting stub for cmpxchg_long which I followed naming 
convention.

   static address _atomic_cmpxchg_entry;
   static address _atomic_cmpxchg_byte_entry;
   static address _atomic_cmpxchg_long_entry;

Technically I think it should be long_long, as well as the 
cmpxchg_long_entry as well.

I also missed renaming store_ptr_entry and add_ptr_entry.  What do you 
suggest?

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

Yes, fixed.  Missed that one.
>
> ---
>
> 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.

It's better as an intx, because that's what it's declared as.   I'll 
patch up some other uses but don't promise total consistency because I 
don't want to pull on this particular sweater thread too much. intx and 
intptr_t I believe are typedefed to each other.

typedef intptr_t  intx;

Should we not have intx and uintx and change all their uses?   I've 
sworn off large changes after this though.

ConstantPoolCacheEntry::make_flags returns an int.   I fixed 
init_flags_atomic() because it's declared with an intx and defined with 
intptr_t.
>
> ---
>
> 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())

Where I introduced it, looked like undefined behavior because it assumed 
that the header was the first field.

So I should sanity check that other places with undefined behavior won't 
break?  Sure I'll do that.
>
> ---
>
> 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.

I didn't see why not and it avoided a bunch of ugly casts.   I tested 
that the SA was fine with it because the SA manually did the address 
adjustment.  The SA could be fixed to know about PaddedEnd if it's 
somehting they want to do.

Thanks for going through and reviewing all of this.   Please answer 
question about the stub function name and I'll include the change with 
this patch.

Coleen

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