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