RFR: 8186476: Generalize Atomic::add with templates

[Erik Österlund]

Hi Kim,

Thank you for finding an approach that seems to have reached consensus, 
and rolling the ball forward while I was on vacation.
I agree this seems to be a reasonable way of doing things and am 
delighted about the general approach.

When it comes to the proposed Atomic::add implementation, it seems to me 
like the emulation of Atomic::add on jshort using jint is very similar 
to how Atomic::cmpxchg on jbyte is emulated using jint. So it surprised 
me a bit that Atomic::add of jshort does not share the same mechanism as 
the generalized Atomic::cmpxchg of jbyte. In fact, it seems like the 
proposed implementation does not safely handle CV-qualifiers. For 
example, if the addend passed in is const qualified, I believe it will 
not match the AddImpl<jshort, jshort> class specialization, as it is 
looking for AddImpl<const jshort, jshort> instead.

Therefore, I would propose dealing with Atomic::add on jshort in one of 
the following ways (numbered according to my preferences):

1) Remove it. Atomic::add on jshort is only ever used by a reference 
counter in the Symbol class that can be placed next to the length. I 
would be surprised if the memory savings of using short instead of int 
for the reference counter is considerable.
2) Implement it like jbyte cmpxchg. That would entail both calling a 
generalized AddShortUsingInt function object from the platform layer, 
and preferrably also using an int CAS loop for emulating the add 
operation rather than the current emulation using atomic int add, that 
relies on two short fields being placed next to each other, depending on 
endianness of the machine, using the ATOMIC_SHORT_PAIR macro for 
declaring the short pair, that may or may not in fact enforce the 
intended alignment at compile time.
3) Continue emulating jshort Atomic::add with jint Atomi::add, but turn 
it into an AddShortUsingInt function object, called from the platform 
layer, like the similar CmpxchgByteUsingInt operation.

So, I am curious if anyone would have loud objections if I was to remove 
Atomic::add support for jshort, and changed its single use 
(Symbol::_refcount), to use int instead. And if there are such 
objections, I wonder if we really want to continue using the 
ATOMIC_SHORT_PAIR macro and emulation using jint Atomic::add, or it is 
okay to rewrite it to use an cmpxch loop instead and get rid of the 
ATOMIC_SHORT_PAIR macro (that I find makes for a very weird API).

Thanks,
/Erik

On 2017-08-20 08:19, Kim Barrett wrote:
>> On Aug 20, 2017, at 2:16 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>
>> Please review this change to Atomic::add, making it a function
>> template with a per-platform underlying implementation.
>>
>> This change is similar to 8186166: "Generalize Atomic::cmpxchg with
>> templates", with similar goals and taking a similar approach.
>>
>> […]
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8186476
>>
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8186476/hotspot.00/
>>
>> Testing:
>> Ad hoc hotspot nightly test.
> I forgot to mention that this change is based on the 8186166 hotspot.04 change,
> which has not yet been pushed.  I’m leaving that to Erik, as well as handing this
> change off to him.
>