RFR: 8186476: Generalize Atomic::add with templates

[David Holmes]

On 26/08/2017 1:28 AM, Erik Österlund wrote:
> Hi Coleen,
> 
> On 2017-08-25 00:17, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 8/21/17 5:17 PM, Kim Barrett wrote:
>>>> On Aug 21, 2017, at 11:57 AM, Erik Österlund 
>>>> <erik.osterlund at oracle.com> wrote:
>>>>> 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.
>>> No, I misremembered.  Yes, that change would cost an average of 2 
>>> additional bytes per symbol, assuming symbol name lengths mod 8 are 
>>> fairly uniformly distributed, which seems likely.
>>>
>>> That’s still 2 extra bytes per symbol, and I doubt the runtime folks 
>>> would like that, since they’ve been going to some trouble to reduce 
>>> the size of symbols:
>>> 8009575: Reduce Symbol::_refcount from 4 bytes to 2 bytes
>>> 8087143: Reduce Symbol::_identity_hash to 2 bytes
>>
>> Yes, the runtime folks would not like that after going through the 
>> trouble to reduce the size of Symbol.  There can be lots of symbols.
> 
> Thank you for clarifying that.  Perhaps coming from GC, my view on this 
> memory optimization is different. But I will respect your view and not 
> question this decision.
> In that case, I see no current objections to the solution Kim handed off 
> to me. Are we good to go?

I think you are good to go. Though I did have some queries buried in my 
earlier response, in particular in relation to the change in 
atomic_solaris_sparc.hpp to use a C++ cas-loop I queried:

"Makes me wonder whether this version should not be in platform 
independent code ready to be inherited by a platform if needed? "

If that is possible/reasonable it can still be deferred to the next phase.

Thanks,
David
-----


> Thanks,
> /Erik
> 
>>
>> Coleen
>>
>>>
>>>>>> 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.
>>> Okay.
>>>
>>
>