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

[coleen.phillimore at oracle.com]

On 9/5/17 8:09 AM, Erik Österlund wrote:
> Hi Kim,
>
> On 2017-09-05 12:31, Kim Barrett wrote:
>>> On Sep 5, 2017, at 10:55 AM, Erik Österlund 
>>> <erik.osterlund at oracle.com> wrote:
>>> On 2017-09-05 02:38, Kim Barrett wrote:
>>>> […] Erik's proposal also uses IntegralContant for that purpose,
>>>> but (from a quick skim) I think got that part wrong, and that is a
>>>> source of additional complexity. Erik might want to re-read my handoff
>>>> email to him.  I don't know whether that approach would satisfy folks
>>>> though.
>>> Unfortunately that approach did not work. It passed IntegralConstant 
>>> as rvalue to operator(), which is illegal as IntegralConstant is an 
>>> AllStatic class.
>> IntegralConstant is an AllStatic?  That’s just wrong!  And probably 
>> my fault too.
>
> It is indeed.
>
>> // A type n is a model of Integral Constant if it meets the following
>> // requirements:
>> //
>> […]
>> // n::value_type const c = n() : c == n::value
>
> Fair point. That should probably be fixed if somebody wants to pass 
> around IntegralConstant by value in some later RFE.
>
>>
>>>> I'm a little bit reluctant to just give up on per-platform
>>>> microoptimized inc/dec and simply transform those operations into
>>>> corresponding add operations.  Requiring an additional register and
>>>> it's initialization for some platforms seems like poor form.
>>> If we insist on having these micro optimizations, I am not opposed 
>>> to on selected x86 GCC platforms (were the risk for future ABI 
>>> breakage is nearly zero) using GCC intrinsics for Atomic::add. If 
>>> Atomic::inc/dec simply calls Atomic::add, then those GCC intrinsics 
>>> will be able to micro optimize as you desire to the optimal lock add 
>>> encoding.
>> I think I’d be okay with that too.
>
> Okay, great. So far it sounds like as for Atomic::inc/dec, there are 
> no loud voices against the idea of removing the Atomic::inc/dec 
> specializations. So I propose this new webrev that does exactly that.
>
> Full webrev:
> http://cr.openjdk.java.net/~eosterlund/8186838/webrev.01/
>
> Incremental over last webrev:
> http://cr.openjdk.java.net/~eosterlund/8186838/webrev.00_01/
>
> I hope this looks simpler.

Erik,

This looks a ton better to me.   With the simplification, the template 
parameter's being D rather than T don't cause as much of a headache.

+template<typename D>
+inline void Atomic::inc(D volatile* dest) {
+ STATIC_ASSERT(IsPointer<D>::value || IsIntegral<D>::value);
+ typedef typename Conditional<IsPointer<D>::value, ptrdiff_t, D>::type T;
+ Atomic::add(T(1), dest);
+}
+
+template<typename D>
+inline void Atomic::dec(D volatile* dest) {
+ STATIC_ASSERT(IsPointer<D>::value || IsIntegral<D>::value);
+ typedef typename Conditional<IsPointer<D>::value, ptrdiff_t, D>::type T;
+ // Assumes two's complement integer representation.
+ #pragma warning(suppress: 4146)
+ Atomic::add(T(-1), dest);
+}
+

It's taking me too long to parse the metaprogramming trick with T.    
What does this do?   Please write a one line comment.  I am guessing 
that this is going to scale the pointer case by the size of the pointed 
to value.  So for pointer it becomes:

Atomic::add(ptrdiff_t(1), dest)) ?  this doesn't do that ?

Thank you for making this simpler.  I agree with Andrews assessment 
about accidental complexity.  This seems like something that shouldn't 
be difficult.

Lastly, does this deprecate inc_ptr()?   After this change, can you file 
an RFE?

Coleen


>
> Thanks,
> /Erik