RFR (L, but tedious) 8188220: Remove Atomic::*_ptr() uses and overloads from hotspot
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Thu Oct 12 12:55:56 UTC 2017
On 10/12/17 8:21 AM, David Holmes wrote:
> On 12/10/2017 9:52 PM, coleen.phillimore at oracle.com wrote:
>> On 10/12/17 3:23 AM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> Thanks for doing this tedious cleanup!
>>>
>>> It was good to see so many casts disappear; and sad to see so many
>>> have to now appear in the sync code. :(
>>
>> The sync code has _owner field as void* because it can be several
>> things. I didn't try to
>
> Yeah I understood why this had to happen.
>
>>>
>>> There were a few things that struck me ...
>>>
>>> Atomic::xchg_ptr turned into Atomic::xchg; yet for the stub
>>> generator routines atomic_xchg_ptr became atomic_xchg_long - but I
>>> can't see where that stub will now come into play?
>>
>> http://cr.openjdk.java.net/~coleenp/8188220.02/webrev/src/hotspot/cpu/x86/stubGenerator_x86_64.cpp.udiff.html
>>
>>
>> I tried to remove it but windows x64 uses a stub for xchg (and others).
>
> Ah so I think this is where it is used:
>
> ./os_cpu/windows_x86/atomic_windows_x86.hpp:DEFINE_STUB_XCHG(8, jlong,
> os::atomic_xchg_ptr_func)
>
> ie atomic_xchg_ptr is the stub for Atomic::xchg<8>
>
>> There was a preexisting stub for cmpxchg_long which I followed naming
>> convention.
>>
>> static address _atomic_cmpxchg_entry;
>> static address _atomic_cmpxchg_byte_entry;
>> static address _atomic_cmpxchg_long_entry;
>>
>> Technically I think it should be long_long, as well as the
>> cmpxchg_long_entry as well.
>
> Or int64_t
>
>> I also missed renaming store_ptr_entry and add_ptr_entry. What do
>> you suggest?
>
> store_ptr_entry actually seems unused.
>
> add_ptr_entry looks like it needs to be the 64-bit Atomic::add<8>
> implementation - so probably add_int64_t_entry.
https://bugs.openjdk.java.net/browse/JDK-8186903
I'm renaming to ptr => long for now to follow other code and fixing the
name with this RFE to what it really is, and what we decide.
It was pretty ugly as:
static jint (*atomic_add_func) (jint, volatile
jint*);
static intptr_t (*atomic_add_ptr_func) (intptr_t, volatile
intptr_t*);
When the other uses jint as an argument. Actually, I think add_ptr
makes more sense in this context than long. I think I should leave this
name and not make it long.
>
>>>
>>> ---
>>>
>>> src/hotspot/share/gc/shared/taskqueue.inline.hpp
>>>
>>> + return (size_t) Atomic::cmpxchg((intptr_t)new_age._data,
>>> + (volatile intptr_t *)&_data,
>>> + (intptr_t)old_age._data);
>>>
>>> The actual types here should be size_t, can we now change it to use
>>> the real type?
>>
>> Yes, fixed. Missed that one.
>>>
>>> ---
>>>
>>> src/hotspot/share/oops/cpCache.cpp
>>>
>>> 114 bool ConstantPoolCacheEntry::init_flags_atomic(intptr_t flags) {
>>> 115 intptr_t result = Atomic::cmpxchg(flags, &_flags, (intptr_t)0);
>>> 116 return (result == 0);
>>> 117 }
>>>
>>> _flags is actually intx, yet above we treat it as intptr_t. But then
>>> later:
>>>
>>> 156 if (_flags == 0) {
>>> 157 intx newflags = (value & parameter_size_mask);
>>> 158 Atomic::cmpxchg(newflags, &_flags, (intx)0);
>>> 159 }
>>>
>>> its intx again. This looks really odd to me.
>>
>> It's better as an intx, because that's what it's declared as. I'll
>> patch up some other uses but don't promise total consistency because
>> I don't want to pull on this particular sweater thread too much. intx
>> and intptr_t I believe are typedefed to each other.
>>
>> typedef intptr_t intx;
>>
>> Should we not have intx and uintx and change all their uses? I've
>> sworn off large changes after this though.
>
> I don't know why we have intx/uintx other than someone not liking
> having to type intptr_t all the time.
>
>> ConstantPoolCacheEntry::make_flags returns an int. I fixed
>> init_flags_atomic() because it's declared with an intx and defined
>> with intptr_t.
>
> Ok.
>
>>>
>>> ---
>>>
>>> src/hotspot/share/runtime/objectMonitor.inline.hpp
>>>
>>> The addition of header_addr() made me a little nervous :) Can we add
>>> a sanity assert either inside it (or in synchronizer.cpp), to verify
>>> that this == &_header (or monitor == monitor->header_addr())
>>
>> Where I introduced it, looked like undefined behavior because it
>> assumed that the header was the first field.
>
> Assumes and expects, I think. Not sure if it is undefined behaviour or
> not.
Assumes without giving the static compiler a chance to check that what
you've done is correct or not. Maybe that's not undefined behavior.
>
>> So I should sanity check that other places with undefined behavior
>> won't break? Sure I'll do that.
>
> No only sanity check that your change actually didn't change anything. :)
As well.
>
>>>
>>> ---
>>>
>>> src/hotspot/share/runtime/synchronizer.cpp
>>>
>>> // global list of blocks of monitors
>>> -// gBlockList is really PaddedEnd<ObjectMonitor> *, but we don't
>>> -// want to expose the PaddedEnd template more than necessary.
>>> -ObjectMonitor * volatile ObjectSynchronizer::gBlockList = NULL;
>>> +PaddedEnd<ObjectMonitor> * volatile ObjectSynchronizer::gBlockList
>>> = NULL;
>>>
>>> Did this have to change? I'm not sure why we didn't want to expose
>>> PaddedEnd, but it is now being exposed.
>>
>> I didn't see why not and it avoided a bunch of ugly casts. I tested
>> that the SA was fine with it because the SA manually did the address
>> adjustment. The SA could be fixed to know about PaddedEnd if it's
>> somehting they want to do.
>
> Glad you mentioned SA as I forgot to mention that with the vmStructs
> changes. :)
>
>> Thanks for going through and reviewing all of this. Please answer
>> question about the stub function name and I'll include the change
>> with this patch.
>
> Would like to see an incremental webrev please. (Should be easy if
> you're using mq :) )
Will do.
Thanks,
Coleen
>
> Thanks,
> David
>
>> Coleen
>>
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>
>>> On 11/10/2017 11:50 PM, 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