RFR (L, but tedious) 8188220: Remove Atomic::*_ptr() uses and overloads from hotspot
David Holmes
david.holmes at oracle.com
Thu Oct 12 21:56:24 UTC 2017
On 13/10/2017 3:23 AM, coleen.phillimore at oracle.com wrote:
>
> Here's the qseries in webrevs.
Are these the latest or do they match the big webrev you previously put out?
> open webrev at http://cr.openjdk.java.net/~coleenp/8188220.add_ptr/webrev
There are still two add(-n) instead of sub(n) cases.
Also here:
--- old/src/hotspot/share/services/mallocTracker.hpp 2017-10-12
12:15:32.951573341 -0400
+++ new/src/hotspot/share/services/mallocTracker.hpp 2017-10-12
12:15:32.386616320 -0400
@@ -68,7 +68,7 @@
if (sz > 0) {
// unary minus operator applied to unsigned type, result still
unsigned
#pragma warning(suppress: 4146)
- Atomic::add(-sz, &_size);
+ Atomic::sub(sz, &_size);
You should be able to remove the comment and pragma now as no unary
minus is being applied (at this level).
Thanks,
David
> open webrev at
> http://cr.openjdk.java.net/~coleenp/8188220.cmpxchg_ptr/webrev
> open webrev at
> http://cr.openjdk.java.net/~coleenp/8188220.cmpxchg_if_null/webrev
> open webrev at http://cr.openjdk.java.net/~coleenp/8188220.xchg_ptr/webrev
> open webrev at http://cr.openjdk.java.net/~coleenp/8188220.store_ptr/webrev
> open webrev at
> http://cr.openjdk.java.net/~coleenp/8188220.load_ptr_acquire/webrev
> open webrev at
> http://cr.openjdk.java.net/~coleenp/8188220.assembler_cmpxchg/webrev
> open webrev at http://cr.openjdk.java.net/~coleenp/8188220.casptr/webrev
> open webrev at
> http://cr.openjdk.java.net/~coleenp/8188220.review-comments/webrev
>
> assembler_cmpxchg should be release_store_ptr which got qrefreshed with
> trying to get the cmpxchg function pointer to compile.
>
> Thanks,
> Coleen
>
> On 10/12/17 8:55 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> 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