RFR (L, but tedious) 8188220: Remove Atomic::*_ptr() uses and overloads from hotspot

Mon Oct 16 13:13:52 UTC 2017

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