RFR: 8234563: Harmonize parameter order in Atomic

Fri Nov 22 11:14:59 UTC 2019

On 2019-11-22 06:15, David Holmes wrote:
> Hi Stefan,
> 
> On 21/11/2019 9:26 pm, Stefan Karlsson wrote:
>> Hi all,
>>
>> I'd like to propose a restructuring of the parameter order in Atomic.
> 
> Okay - consistency is good, especially if it then aligns with external 
> APIs. Thanks for doing this cleanup!
> 
>> https://bugs.openjdk.java.net/browse/JDK-8234563
>>
>> These are some of the functions used to concurrently write memory from 
>> HotSpot C++ code:
>>
>>   Atomic::store(value, destination);
>>   OrderAccess::release_store(destination, value)
>>   OrderAccess::release_store_fence(destination, value)
>>   Atomic::add(value, destination);
>>   Atomic::sub(value, destination);
>>   Atomic::xchg(exchange_value, destination);
>>   Atomic::cmpxchg(exchange_value, destination, compare_value);
>>
>> With the proposed JDK-8234562 change, this would look like:
>>
>>   Atomic::store(value, destination);
>>   Atomic::release_store(destination, value)
>>   Atomic::release_store_fence(destination, value)
>>   Atomic::add(value, destination);
>>   Atomic::sub(value, destination);
>>   Atomic::xchg(exchange_value, destination);
>>   Atomic::cmpxchg(exchange_value, destination, compare_value);
>>
>> I'd like to propose that we move the destination parameter over to the 
>> left, and the new value to the right. This would look like this:
>>
>>   Atomic::store(destination, value);
>>   Atomic::release_store(destination, value)
>>   Atomic::release_store_fence(destination, value)
>>   Atomic::add(destination, value);
>>   Atomic::sub(destination, value);
>>   Atomic::xchg(destination, exchange_value);
>>   Atomic::cmpxchg(destination, compare_value, exchange_value);
> 
> I was expecting
> 
> Atomic::cmpxchg(destination, exchange_value, compare_value);
> 
> as that would seem more consistent so that we always have:
> - arg 1 => destination
> - arg 2 => new (or delta) value
> 
> but I guess this is consistent with external cmpxchg APIs :(
> 
>> This would bring the Atomic API more in-line with the order for a 
>> normal store:
>>
>> *destination = value;
>>
>> I've split this up into separate patches, each dealing with a separate 
>> operation:
>>
>> Atomic::store:
>>   https://cr.openjdk.java.net/~stefank/8234563/webrev.01.01.store/
> 
> Ok.
> 
>> Atomic::add:
>>   https://cr.openjdk.java.net/~stefank/8234563/webrev.01.02.add/
> 
> src/hotspot/os_cpu/solaris_x86/solaris_x86_64.il
> 
> You updated the comments to show the parameters have changed order but 
> the assembly code is unchanged - won't it need modification too? Or is 
> it that the true call site for this still uses the original order ??
> 

Right. I've changed the order for the C++ functions, but left inline 
assembly that assumes a given parameter order. I didn't want to change 
that with this patch. Here's where the arguments are swapped:

// Not using add_using_helper; see comment for cmpxchg.
template<>
template<typename D, typename I>
inline D Atomic::PlatformAdd<4>::add_and_fetch(D volatile* dest, I 
add_value,
                                                atomic_memory_order 
order) const {
   STATIC_ASSERT(4 == sizeof(I));
   STATIC_ASSERT(4 == sizeof(D));
   return PrimitiveConversions::cast<D>(
     _Atomic_add(PrimitiveConversions::cast<int32_t>(add_value),
                 reinterpret_cast<int32_t volatile*>(dest)));
}

So, the comment refers to the high-level Atomic::add function, not the 
_Atomic_add implementation. I agree that it's confusing, and thought 
about that, but I don't have a good suggestion how to change these 
comments. If you have a good suggestion on what to changes this to, I 
can update all these comments.

> -- 
> 
> I was a bit confused about the changes involving Atomic::add_using_helper:
> 
> +template<typename Type, typename Fn, typename D, typename I>
> +inline D Atomic::add_using_helper(Fn fn, D volatile* dest, I add_value) {
>     return PrimitiveConversions::cast<D>(
>       fn(PrimitiveConversions::cast<Type>(add_value),
>          reinterpret_cast<Type volatile*>(dest)));
> 
> because you switched the Atomic API parameter order, but the underlying 
> function being called still has the parameters passed in the old order. 
> I'm not sure whether this is the right level to stop the change or 
> whether it should have been pushed all the way down to e.g. 
> os::atomic_add_func ?

If I wanted to push this further down I would have to start to change 
assembly code that relied on a specific order. That's why I chose to 
stop here.

See:
add_func_t*          os::atomic_add_func          = 
os::atomic_add_bootstrap;

---

int32_t os::atomic_add_bootstrap(int32_t add_value, volatile int32_t* 
dest) { 

   // try to use the stub: 

   add_func_t* func = CAST_TO_FN_PTR(add_func_t*, 
StubRoutines::atomic_add_entry()); 

   if (func != NULL) { 

     os::atomic_add_func = func; 

     return (*func)(add_value, dest); 

   } 

   assert(Threads::number_of_threads() == 0, "for bootstrap only"); 

   return (*dest) += add_value; 

}

---

StubRoutines::_atomic_add_entry           = generate_atomic_add();

---

   address generate_atomic_add() { 

     StubCodeMark mark(this, "StubRoutines", "atomic_add"); 

     address start = __ pc(); 

     __ movl(rax, c_rarg0); 

     __ lock(); 

     __ xaddl(Address(c_rarg1, 0), c_rarg0); 

     __ addl(rax, c_rarg0); 

     __ ret(0); 

     return start; 

   }

I'd prefer to leave that exercise to someone else.

> 
> Otherwise seems okay.
> 
>> Atomic::sub:
>>   https://cr.openjdk.java.net/~stefank/8234563/webrev.01.03.sub/
> 
> Ok.
> 
>> Atomic::xchg:
>>   https://cr.openjdk.java.net/~stefank/8234563/webrev.01.04.xchg/
> 
> src/hotspot/os_cpu/solaris_x86/solaris_x86_64.il
> 
> same issue with the assembly as per Atomic::add.
> 
> ---
> 
> Same issue/query with xchg_using_helper as add_using_helper.
> 
>> Atomic::cmpxchg:
>>   https://cr.openjdk.java.net/~stefank/8234563/webrev.01.05.cmpxchg/
> 
> src/hotspot/os_cpu/bsd_x86/bsd_x86_32.s
> src/hotspot/os_cpu/linux_x86/linux_x86_32.s
> src/hotspot/os_cpu/solaris_x86/solaris_x86_64.il
> 
> Again queries about the actual assembly code.

Thanks for reviewing this,
StefanK

> 
> Thanks,
> David
> -----
> 
>> All sub-patches combined:
>>   https://cr.openjdk.java.net/~stefank/8234563/webrev.01.all/
>>
>> The patches applies on-top of the patch for JDK-8234562:
>>   https://cr.openjdk.java.net/~stefank/8234562/webrev.01/
>>
>> I've tested this patch with tier1-7. I've also built fastdebug on the 
>> following configs: aarch64, arm32, ppc64le, s390x, shenandoah, zero, 
>> minimal, just to minimize any disruptions. However, this is a moving 
>> target and latest rebase of this has only been run on tier1.
>>
>> Thanks,
>> StefanK