RFR (M): 8187977: Generalize Atomic::xchg to use templates

Fri Sep 29 19:17:08 UTC 2017

Erik, This change looks good to me.

Do we want to deprecate xchg_ptr like cmpxchg_ptr ?   Is there an RFE 
filed for that?

On 9/29/17 6:45 AM, Erik Österlund wrote:
> Hi Kim,
>
> Thanks for looking at this.
>
> Incremental webrev:
> http://cr.openjdk.java.net/~eosterlund/8187977/webrev.00_01/
>
> Full webrev:
> http://cr.openjdk.java.net/~eosterlund/8187977/webrev.01/
>
> On 2017-09-28 21:02, Kim Barrett wrote:
>>> On Sep 28, 2017, at 7:56 AM, Erik Österlund 
>>> <erik.osterlund at oracle.com> wrote:
>>>
>>> Hi all,
>>>
>>> The time has come to generalize more atomics.
>>>
>>> I have modelled Atomic::xchg to systematically do what Atomic::cmpxchg
>>> did but for xchg.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8187977
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8187977/webrev.00/
>>>
>>> Testing: mach5 hs-tier3 and JPRT
>>>
>>> Thanks,
>>> /Erik
>> ============================================================================== 
>>
>> src/hotspot/share/runtime/atomic.hpp
>>   312   // A default definition is not provided, so specializations 
>> must be
>>   313   // provided for:
>>   314   //   T operator()(T, T volatile*) const
>>
>> That description is not correct. The default definition of
>> PlatformXchg is at line 410. And it makes no sense to talk about
>> specializing a function for a class that doesn't exist. This should
>> instead be using similar wording to that in the description of
>> PlatformCmpxchg.
>
> Fixed.
>
>> ============================================================================== 
>>
>> src/hotspot/share/runtime/atomic.hpp
>>
>> There are a lot of similarities between the handling of the
>> exchange_value by cmpxchg and xchg, and I expect Atomic::store and
>> OrderAccess::release_store and variants to also be similar. (Actually,
>> xchg and store may look *very* similar.) It would be nice to think
>> about whether there's some refactoring that could be used to reduce
>> the amount of code involved. I don't have a specific suggestion yet
>> though.  For now, this is just something to think about.
>
> I would not like to mix xchg and load/store implementations in Atomic. 
> The reason for that is that I would rather share the implementation 
> between Atomic::load/store and OrderAccess::*load/store*, because they 
> are even more similar. I have that already in my patch queue.
>

I think I would have to see understand what the duplication is before I 
want to see more lines of metaprogramming tricks.

Do you mean the translate recover/decay here?

697 template<typename T>
  698 struct Atomic::XchgImpl<
  699   T, T,
  700   typename EnableIf<PrimitiveConversions::Translate<T>::value>::type>
  701   VALUE_OBJ_CLASS_SPEC
  702 {
  703   T operator()(T exchange_value, T volatile* dest) const {
  704     typedef PrimitiveConversions::Translate<T> Translator;
  705     typedef typename Translator::Decayed Decayed;
  706     STATIC_ASSERT(sizeof(T) == sizeof(Decayed));
  707     return Translator::recover(
  708       xchg(Translator::decay(exchange_value),
  709            reinterpret_cast<Decayed volatile*>(dest)));
  710   }

I can imagine this can only have more levels of logical indirection if 
generalized more.

I have to admit that I hate the VALUE_OBJ_CLASS_SPEC in these classes.   
There's already enough decoder ring work for each line without this 
distracting (mostly useless) macro here.   Originally, this meant that 
this class is embedded in another.  It doesn't really help the code in 
any way here.   Global operator new already has an assert in the very 
unlikely case that somebody did a

Atomic::XchgImpl<int, int>* x = new Atomic::XchgImpl<int, int>();   // 
did I write that correctly?

>> ============================================================================== 
>>
>> src/hotspot/share/compiler/compileBroker.hpp
>>   335     Atomic::xchg(jint(shutdown_compilation), 
>> &_should_compile_new_jobs);
>> [Added jint conversion.]
>>
>> Pre-existing:
>>
>> Why is this an Atomic::xchg at all, since the old value isn't used.
>> Seems like it could be a release_store.
>>
>> There also seems to be some data typing problems around
>> _should_compile_new_jobs.  Shouldn't that variable be a
>> CompilerActivity?  That wouldn't have worked previously, and we're
>> probably still not ready for it if the xchg gets changed to a
>> release_store, but eventually... so there probably ought to be a bug
>> for it.
>
> I will file a bug for that to be reconsidered later on.
>

I would leave this to the compiler group to look at.

Thanks,
Coleen

>> ============================================================================== 
>>
>> src/hotspot/os_cpu/solaris_x86/atomic_solaris_x86.hpp
>>   146 template<>
>>   147 template<typename T>
>>   148 inline T Atomic::PlatformXchg<8>::operator()(T exchange_value,
>>
>> Please move this definition up near the PlatformXchg<4> definition,
>> e.g. around line 96.
>
> Fixed.
>
>>
>> ============================================================================== 
>>
>> src/hotspot/os_cpu/linux_arm/atomic_linux_arm.hpp
>>
>> I would prefer the two PlatformXchg specializations be adjacent.  If
>> the 4byte specialization was after PlatformAdd<8>, reviewing would
>> have been easier, and the different Add would be adjacent and the
>> different Xchg would be adjacent.  The only cost is an extra #ifdef
>> AARCH64 block around the 8byte Xchg.
>
> Fixed.
>
>> ============================================================================== 
>>
>> src/hotspot/os_cpu/bsd_zero/atomic_bsd_zero.hpp
>>   216   return xchg_using_helper<int>(arm_lock_test_and_set, 
>> exchange_value, dest);
>>   219   return xchg_using_helper<int>(m68k_lock_test_and_set, 
>> exchange_value, dest);
>>
>> arm/m68k_lock_test_and_set expect their arguments in the other order,
>> and need to be updated.  There was a similar change for cmpxchg.
>>
>> Same problem in atomic_linux_zero.hpp.
>
> Fixed.
>
> Thanks for the review.
>
> /Erik