RFR (L, but tedious) 8188220: Remove Atomic::*_ptr() uses and overloads from hotspot
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Oct 17 00:45:03 UTC 2017
Thanks David!
Coleen
On 10/16/17 5:58 PM, David Holmes wrote:
> Seems okay.
>
> Thanks,
> David
>
> On 17/10/2017 1:59 AM, coleen.phillimore at oracle.com wrote:
>>
>> The latest incremental based on these comments (now running tier1).
>> http://cr.openjdk.java.net/~coleenp/8188220.review-comments.02/webrev/index.html
>>
>>
>> plus what Roman sent in the "RFR: 8189333: Fix Zero build after
>> Atomic::xchg changes" thread.
>>
>> thanks,
>> Coleen
>>
>> On 10/16/17 9:13 AM, coleen.phillimore at oracle.com wrote:
>>>
>>>
>>> On 10/14/17 7:36 PM, Kim Barrett wrote:
>>>>> On Oct 13, 2017, at 2:34 PM, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>>
>>>>> Hi, Here is the version with the changes from Kim's comments that
>>>>> has passed at least testing with JPRT and tier1, locally. More
>>>>> testing (tier2-5) is in progress.
>>>>>
>>>>> Also includes a corrected version of Atomic::sub care of Erik
>>>>> Osterlund.
>>>>>
>>>>> open webrev at
>>>>> http://cr.openjdk.java.net/~coleenp/8188220.kim-review-changes/webrev
>>>>> open webrev at
>>>>> http://cr.openjdk.java.net/~coleenp/8188220.review-comments/webrev
>>>>>
>>>>> Full version:
>>>>>
>>>>> http://cr.openjdk.java.net/~coleenp/8188220.03/webrev
>>>>>
>>>>> Thanks!
>>>>> Coleen
>>>> I still dislike and disagree with what is being proposed regarding
>>>> replace_if_null.
>>>
>>> We can discuss that seperately, please file an RFE.
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>> I forgot that I'd promised you an updated Atomic::sub definition.
>>>> Unfortunately, the new one still has problems, performing some
>>>> conversions that should not be permitted (and are disallowed by
>>>> Atomic::add). Try this instead. (This hasn't been tested, not even
>>>> compiled; hopefully I don't have any typos or anything.) The intent
>>>> is that this supports the same conversions as Atomic::add.
>>>>
>>>> template<typename I, typename D>
>>>> inline D Atomic::sub(I sub_value, D volatile* dest) {
>>>> STATIC_ASSERT(IsPointer<D>::value || IsIntegral<D>::value);
>>>> STATIC_ASSERT(IsIntegral<I>::value);
>>>> // If D is a pointer type, use [u]intptr_t as the addend type,
>>>> // matching signedness of I. Otherwise, use D as the addend type.
>>>> typedef typename Conditional<IsSigned<I>::value, intptr_t,
>>>> uintptr_t>::type PI;
>>>> typedef typename Conditional<IsPointer<D>::value, PI, D>::type
>>>> AddendType;
>>>> // Only allow conversions that can't change the value.
>>>> STATIC_ASSERT(IsSigned<I>::value == IsSigned<AddendType>::value);
>>>> STATIC_ASSERT(sizeof(I) <= sizeof(AddendType));
>>>> AddendType addend = sub_value;
>>>> // Assumes two's complement integer representation.
>>>> #pragma warning(suppress: 4146) // In case AddendType is not
>>>> signed.
>>>> return Atomic::add(-addend, dest);
>>>> }
>>>
>>> Uh, Ok. I'll try it out.
>>>>
>>>>>>> src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp
>>>>>>> 7960 Atomic::add(-n, &_num_par_pushes);
>>>>>>>
>>>>>>> Atomic::sub
>>>>>> fixed.
>>>> Nope, not fixed in
>>>> http://cr.openjdk.java.net/~coleenp/8188220.03/webrev
>>>
>>> Missed it twice now. I think I have it now.
>>>>>>> src/hotspot/share/gc/g1/heapRegionRemSet.cpp
>>>>>>> 200 PerRegionTable* res =
>>>>>>> 201 Atomic::cmpxchg(nxt, &_free_list, fl);
>>>>>>>
>>>>>>> Please remove the line break, now that the code has been
>>>>>>> simplified.
>>>>>>>
>>>>>>> But wait, doesn't this alloc exhibit classic ABA problems? I
>>>>>>> *think*
>>>>>>> this works because alloc and bulk_free are called in different
>>>>>>> phases,
>>>>>>> never overlapping.
>>>>>> I don't know. Do you want to file a bug to investigate this?
>>>>>> fixed.
>>>> No, I now think it’s ok, though confusing.
>>>>
>>>>>>> src/hotspot/share/gc/g1/sparsePRT.cpp
>>>>>>> 295 SparsePRT* res =
>>>>>>> 296 Atomic::cmpxchg(sprt, &_head_expanded_list, hd);
>>>>>>> and
>>>>>>> 307 SparsePRT* res =
>>>>>>> 308 Atomic::cmpxchg(next, &_head_expanded_list, hd);
>>>>>>>
>>>>>>> I'd rather not have the line breaks in these either.
>>>>>>>
>>>>>>> And get_from_expanded_list also appears to have classic ABA
>>>>>>> problems.
>>>>>>> I *think* this works because add_to_expanded_list and
>>>>>>> get_from_expanded_list are called in different phases, never
>>>>>>> overlapping.
>>>>>> Fixed, same question as above? Or one bug to investigate both?
>>>> Again, I think it’s ok, though confusing.
>>>>
>>>>>>> src/hotspot/share/gc/shared/taskqueue.inline.hpp
>>>>>>> 262 return (size_t) Atomic::cmpxchg((intptr_t)new_age._data,
>>>>>>> 263 (volatile intptr_t
>>>>>>> *)&_data,
>>>>>>> 264 (intptr_t)old_age._data);
>>>>>>>
>>>>>>> This should be
>>>>>>>
>>>>>>> return Atomic::cmpxchg(new_age._data, &_data, old_age._data);
>>>>>> fixed.
>>>> Still casting the result.
>>>
>>> I thought I fixed it. I think I fixed it now.
>>>>
>>>>>>> src/hotspot/share/oops/method.hpp
>>>>>>> 139 volatile address from_compiled_entry() const { return
>>>>>>> OrderAccess::load_acquire(&_from_compiled_entry); }
>>>>>>> 140 volatile address from_compiled_entry_no_trampoline()
>>>>>>> const;
>>>>>>> 141 volatile address from_interpreted_entry() const{ return
>>>>>>> OrderAccess::load_acquire(&_from_interpreted_entry); }
>>>>>>>
>>>>>>> [pre-existing]
>>>>>>> The volatile qualifiers here seem suspect to me.
>>>>>> Again much suspicion about concurrency and giant pain, which I
>>>>>> remember, of debugging these when they were broken.
>>>> Let me be more direct: the volatile qualifiers for the function return
>>>> types are bogus and confusing, and should be removed.
>>>
>>> Okay, sure.
>>>
>>>>
>>>>>>> src/hotspot/share/prims/jni.cpp
>>>>>>>
>>>>>>> [pre-existing]
>>>>>>>
>>>>>>> copy_jni_function_table should be using
>>>>>>> Copy::disjoint_words_atomic.
>>>>>> yuck.
>>>> Of course, neither is entirely technically correct, since both are
>>>> treating conversion of function pointers to void* as okay in shared
>>>> code, e.g. violating some of the raison d'etre of
>>>> CAST_{TO,FROM}_FN_PTR.
>>>> For way more detail than you probably care about, see the discussion
>>>> starting here:
>>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-March/018578.html
>>>>
>>>> through (5 messages in total)
>>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-March/018623.html
>>>>
>>>>
>>>> Oh well.
>>>>
>>>>>>> src/hotspot/share/runtime/mutex.hpp
>>>>>>>
>>>>>>> [pre-existing]
>>>>>>>
>>>>>>> I think the Address member of the SplitWord union is unused.
>>>>>>> Looking
>>>>>>> at AcquireOrPush (and others), I'm wondering whether it *should* be
>>>>>>> used there, or whether just using intptr_t casts and doing integral
>>>>>>> arithmetic (as is presently being done) is easier and clearer.
>>>>>>>
>>>>>>> Also the _LSBINDEX macro probably ought to be defined in mutex.cpp
>>>>>>> rather than polluting the global namespace. And technically, that
>>>>>>> name is reserved word.
>>>>>> I moved both this and _LBIT into the top of mutex.cpp since they
>>>>>> are used there.
>>>> Good.
>>>>
>>>>>> Cant define const intptr_t _LBIT =1; in a class in our version of
>>>>>> C++.
>>>> Sorry, please explain? If you tried to move it into SplitWord,
>>>> that doesn’t work;
>>>> unions are not permitted to have static data members (I don’t
>>>> off-hand know why,
>>>> just that it’s explicitly forbidden).
>>>>
>>>> And you left the seemingly unused Address member in SplitWord.
>>>
>>> This is the compilation error I get:
>>>
>>> /scratch/cphillim/hg/10ptr2/open/src/hotspot/share/runtime/mutex.hpp:124:33:
>>> error: non-static data member initializers only available with
>>> -std=c++11 or -std=gnu++11 [-Werror]
>>> const intptr_t _NEW_LOCKBIT = 1;
>>>
>>>
>>> I don't own this SplitWord code so do not want to remove the unused
>>> Address member.
>>>
>>>>
>>>>>>> src/hotspot/share/runtime/thread.cpp
>>>>>>> 4707 intptr_t w = Atomic::cmpxchg((intptr_t)LOCKBIT, Lock,
>>>>>>> (intptr_t)0);
>>>>>>>
>>>>>>> This and other places suggest LOCKBIT should be defined as
>>>>>>> intptr_t,
>>>>>>> rather than as an enum value. The MuxBits enum type is unused.
>>>>>>>
>>>>>>> And the cast of 0 is another case where implicit widening would
>>>>>>> be nice.
>>>>>> Making LOCKBIT a const intptr_t = 1 removes a lot of casts.
>>>> Because of the new definition of LOCKBIT I noticed the immediately
>>>> preceeding typedef for MutexT, which seems to be unused.
>>>
>>> Removed MutexT.
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>> src/hotspot/share/oops/cpCache.cpp
>>>> 114 bool ConstantPoolCacheEntry::init_flags_atomic(intx flags) {
>>>> 115 intptr_t result = Atomic::cmpxchg(flags, &_flags, (intx)0);
>>>> 116 return (result == 0);
>>>> 117 }
>>>>
>>>> [I missed this on earlier pass.]
>>>>
>>>> Should be
>>>>
>>>> bool ConstantPoolCacheEntry::init_flags_atomic(intx flags) {
>>>> return Atomic::cmpxchg(flags, &_flags, (intx)0) == 0;
>>>> }
>>>>
>>>> Otherwise, I end up asking why result is intptr_t when the cmpxchg is
>>>> dealing with intx. Yeah, one's a typedef of the other, but mixing
>>>> them like that in the same expression is not helpful.
>>>>
>>>>
>>> Sure why not?
>>>
>>> Actually init_flags_atomic is not used and neither is
>>> init_method_flags_atomic so I did one better and removed them.
>>>
>>> Thanks for the again thorough code review and Atomic::sub. I'll post
>>> incremental when it compiles.
>>>
>>> Coleen
>>
More information about the hotspot-dev
mailing list