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

Sat Sep 30 21:02:20 UTC 2017

Hi Coleen,

Thanks for the review. I will file an RFE for removing the _ptr
variants of Atomic.

/Erik

On fre, 2017-09-29 at 15:17 -0400, coleen.phillimore at oracle.com wrote:
> 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