RFR: 8186476: Generalize Atomic::add with templates

[Erik Österlund]

Hi David,

On 2017-08-28 03:51, David Holmes wrote:
> 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.

Thank you for the clarification. I agree that the CAS loop 
implementation of Atomic::add on solaris_sparc looks like it could 
potentially be generalized if needed. But as you say - let's defer that 
until next phase.

Thanks,
/Erik

>
> 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.
>>>>
>>>
>>