RFR: 8186476: Generalize Atomic::add with templates

[Erik Österlund]

Hi Kim,

On 2017-08-21 16:20, Kim Barrett wrote:
>> On Aug 21, 2017, at 9:10 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>
>> 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.
> That’s incorrect.  lvalue-to-rvalue conversion for non-class types drops the const qualifier.

Ah, yes you are right. I suppose that makes passing in const jshort 
lvalues to Atomic::add fine if using template type inference for the 
template parameters. If one was to explicitly state the types as 
Atomic::add<const jshort, jshort>(...), it would not work though. But I 
guess that is okay, as one might argue the user of the API should not be 
doing that. And again, there is a single use case so might not be worth 
the hassle.

>> 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.
> I’m fairly sure the runtime folks would say you should be surprised, and would object to such a change.  The layout of symbol is carefully constructed, and I think such a change would increase the size of a symbol by 6 byes (not the obvious two) on a 64 bit platform.

I find that surprising. By making body jbyte[0] instead, it seems to me 
(after a glance) to be possible to retain the size of the header from 8 
bytes to 8 bytes after turning refcount to an int. This leaves only the 
difference in payload and its effect on the allocator and its alignment, 
which should seemingly not require 6 extra bytes. But perhaps I have 
misunderstood something about the layout of Symbol.

>> 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).
> I don’t think it’s worth spending a lot of effort generalizing the short case right now.  As you noticed, there is *exactly* one use of it, for the symbol refcount.  Reconsider if that ever changes.
>

Okay, I agree. I will drop that for now. This single use case is 
probably not worth the effort.

Thanks,
/Erik