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

[coleen.phillimore at oracle.com]

Hi, I'm trying to parse the templates to review this but maybe it's 
convention but decoding these with parameters that are single capital 
letters make reading the template very difficult.  There are already a 
lot of non-alphanumeric characters.   When the letter is T, that is 
expected by convention, but D or especially I makes it really hard.   
Can these be normalized to all use T when there is only one template 
parameter?  It'll be clear that T* is a pointer and T is an integer 
without having it be P.

+template<typename I>
+struct Atomic::IncImpl<I, typename 
EnableIf<IsIntegral<I>::value>::type> VALUE_OBJ_CLASS_SPEC {
+ void operator()(I volatile* dest) const {
+ typedef IntegralConstant<I, I(1)> Adjustment;
+ typedef PlatformInc<sizeof(I), Adjustment> PlatformOp;
+ PlatformOp()(dest);
+ }
+};

This one isn't as difficult, because it's short, but it would be faster 
to understand with T.

+template<typename T>
+struct Atomic::IncImpl<T, typename 
EnableIf<IsIntegral<I>::value>::type> VALUE_OBJ_CLASS_SPEC {
+ void operator()(T volatile* dest) const {
+ typedef IntegralConstant<T, T(1)> Adjustment;
+ typedef PlatformInc<sizeof(T), Adjustment> PlatformOp;
+ PlatformOp()(dest);
+ }
+};

+template<>
+struct Atomic::IncImpl<jshort> VALUE_OBJ_CLASS_SPEC {
+ void operator()(jshort volatile* dest) const {
+ add(jshort(1), dest);
+ }
+};


Did I already ask if this could be changed to u2 rather than jshort?  Or 
is that the follow-on RFE?

+// Helper for platforms wanting a constant adjustment.
+template<size_t byte_size, typename Adjustment>
+struct Atomic::IncUsingConstant VALUE_OBJ_CLASS_SPEC {
+ typedef PlatformInc<byte_size, Adjustment> Derived;


I can't find the caller of this.  Is it really a lot faster than having 
the platform independent add(1, T) / add(-1, T) to make all this code 
worth having?  How is this called?  I couldn't parse the trick.  
Atomic::inc() is always a "constant adjustment" so I'm confused about 
what the comment means and what motivates all the asm code.   Do these 
platform implementations exist because they don't have twos complement 
for integer representation?  really?

Also, the function name This() is really disturbing and distracting.  
Can it be called some verb() representing what it does?  cast_to_derived()?

+ template<typename I>
+ void operator()(I volatile* dest) const {
+ This()->template inc<I, Adjustment>(dest);
+ }


I didn't know you could put "template" there.   What does this call?

Rather than I for integer case, and P for pointer case, can you add a 
one line comment above this like:
// Helper for integer types
and
// Helper for pointer types

Small local comments would be really helpful for many of these 
functions.   Just to get more english words in there...  Since Kim's on 
vacation can you help me understand this code and add comments so I 
remember the reasons for some of this?

Thanks!
Coleen


On 8/31/17 8:45 AM, 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.
>
> It is worth noticing that the generalized Atomic::dec operation 
> assumes a two's complement integer machine and potentially sends the 
> unary negative of a potentially unsigned type to Atomic::add. I have 
> the following comments about this:
> 1) We already assume in other code that two's complement integers must 
> be present.
> 2) A machine that does not have two's complement integers may still 
> simply provide a specialization that solves the problem in a different 
> way.
> 3) The alternative that does not make assumptions about that would use 
> the good old IntegerTypes::cast_to_signed metaprogramming stuff, and I 
> seem to recall we thought that was a bit too involved and complicated.
> This is the reason why I have chosen to use unary minus on the 
> potentially unsigned type in the shared helper code that sends the 
> decrement as an addend to Atomic::add.
>
> It would also be nice if somebody with access to PPC and s390 machines 
> could try out the relevant changes there so I do not accidentally 
> break those platforms. I have blind-coded the addition of the 
> immediate values passed in to the inline assembly in a way that I 
> think looks like it should work.
>
> Testing:
> RBT hs-tier3, JPRT --testset hotspot
>
> Thanks,
> /Erik