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

[coleen.phillimore at oracle.com]

On 10/11/17 11:36 AM, Erik Österlund wrote:
> Hi Coleen,
>
> In classLoaderData.cpp:~167:
> There is a cast to Chunk* when loading _head, but _head is already 
> Chunk*, so it seems like that should not need a cast. In fact, _head 
> should probably be declared as Chunk *volatile as it is accessed 
> concurrently.

Yes, you are right.  I fixed it and now declare _head as Chunk* volatile 
(star goes on type I think).
>
> In parNewGeneration.cpp:~1450:
> Atomic::add(-n, &_num_par_pushes);
> can now use Atomic::sub instead.

Fixed.
>
> g1PageBasedVirtualSpace.cpp:~249:
> Do you really need the (char*) cast for Atomic::add? Seems like it 
> already is a char*, unless I missed something.
>

Nope.  Missed that one.

> cpCache.hpp:
> Noticed the casts for &_f1 (declared as volatile Metadata*) to 
> Metadata *volatile*. It seems to me like _f1 should instead be 
> declared as Metaata* volatile, and remove the casts.
>

Fixed.  You are right about the declaration for _f1.  It should be 
Metadata* volatile.
> Also noticed some copyright headers have not been updated, might want 
> to have a look at that.
>

I forgot to say that I update the copyrights in my commit script.

> Otherwise, I think this looks good. Thank you again for doing this!
>

Thank you so much for reviewing all of this and making the templates 
easy to use.

Coleen

> Thanks,
> /Erik
>
> On 2017-10-11 15:50, 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.
>>>>>>>>
>>>>>>
>>>
>>
>