RFR: 8186838: Generalize Atomic::inc/dec with templates

[Andrew Haley]

On 31/08/17 13:45, Erik Österlund wrote:
> Hi everyone,
> 
> Bug ID:
> https://bugs.openjdk.java.net/browse/JDK-8186838
> 
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8186838/webrev.00/
> 
> The time has come for the next step in generalizing Atomic with 
> templates. Today I will focus on Atomic::inc/dec.
> 
> I have tried to mimic the new Kim style that seems to have been 
> universally accepted. Like Atomic::add and Atomic::cmpxchg, the 
> structure looks like this:
> 
> Layer 1) Atomic::inc/dec calls an IncImpl()/DecImpl() function object 
> that performs some basic type checks.
> Layer 2) IncImpl/DecImpl calls PlatformInc/PlatformDec that can define 
> the operation arbitrarily for a given platform. The default 
> implementation if not specialized for a platform is to call Atomic::add. 
> So only platforms that want to do something different than that as an 
> optimization have to provide a specialization.
> Layer 3) Platforms that decide to specialize PlatformInc/PlatformDec to 
> be more optimized may inherit from a helper class 
> IncUsingConstant/DecUsingConstant. This helper helps performing the 
> necessary computation what the increment/decrement should be after 
> pointer scaling using CRTP. The PlatformInc/PlatformDec operation then 
> only needs to define an inc/dec member function, and will then get all 
> the context information necessary to generate a more optimized 
> implementation. Easy peasy.

I wanted to say something nice, but I honestly can't.  I am dismayed.

I hoped that inc/dec would turn out to be much simpler than the
cmpxchg functions: I think they should, because they don't have to
deal with the complexity of potentially three different types.
Instead we have, again, a large and complex patch.

Even on AArch64, which should be the simplest case because Atomic::inc
can be defined as

template <typename T1>
inc(T1 *dest) {
  return __sync_add_and_fetch(dest, 1);
}

or something similar, we have

Atomic::inc<int>
Atomic::IncImpl<int, void>::operator()
Atomic::PlatformInc<4ul, IntegralConstant<int, 1> >::operator()
Atomic::add<int, int>
Atomic::AddImpl<int, int, void>::operator()
Atomic::AddAndFetch<Atomic::PlatformAdd<4ul> >::operator()<int, int>
Atomic::PlatformAdd<4ul>::add_and_fetch<int, int>
__sync_add_and_fetch

I quite understand that it isn't so easy on some systems, and they
need a generic form that explodes into four different calls, one for
each size of integer.  I completely accept that it will be more
complex for everything else.  But is it necessary to have so much code
for something so simple?  This is a 1400 line patch.  Granted, much of
it is simply moving stuff around, but despite the potential of
template code to simplify the implementation we have a more complex
solution than we had before.

I ask you, is this the simplest solution that you believe is possible?

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671