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

[Erik Österlund]

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.

In parNewGeneration.cpp:~1450:
Atomic::add(-n, &_num_par_pushes);
can now use Atomic::sub instead.

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

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.

Also noticed some copyright headers have not been updated, might want to 
have a look at that.

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

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