RFR (L, but tedious) 8188220: Remove Atomic::*_ptr() uses and overloads from hotspot
David Holmes
david.holmes at oracle.com
Mon Oct 16 21:58:01 UTC 2017
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