RFR (L, but tedious) 8188220: Remove Atomic::*_ptr() uses and overloads from hotspot
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Oct 16 15:59:45 UTC 2017
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