RFR (L, but tedious) 8188220: Remove Atomic::*_ptr() uses and overloads from hotspot
David Holmes
david.holmes at oracle.com
Thu Oct 12 12:21:36 UTC 2017
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.
>>
>> ---
>>
>> 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.
> 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. :)
>>
>> ---
>>
>> 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 :) )
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