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 17:44:34 UTC 2017

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.

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


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

More information about the hotspot-dev mailing list