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

Erik Österlund erik.osterlund at oracle.com
Wed Oct 11 15:36:04 UTC 2017

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 

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

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

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!


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.

More information about the hotspot-dev mailing list