Here's the qseries in webrevs.
open webrev at http://cr.openjdk.java.net/~coleenp/8188220.add_ptr/webrev
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.
>>>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>>>
>>>
>