RFR (S) 8009575 (2nd) - Reduce Symbol::_refcount from 4 bytes to 2 bytes

[Daniel D. Daugherty]

On 6/20/13 6:39 PM, Coleen Phillimore wrote:
>
> On 06/20/2013 08:27 PM, David Holmes wrote:
>> Hi Ioi,
>>
>> Reviewed. But see below.
>>
>> On 21/06/2013 4:20 AM, Ioi Lam wrote:
>>> Please review my 2nd trial for this bug fix. It's much simplified 
>>> thanks
>>> to Coleen's suggestion.
>>>
>>> http://cr.openjdk.java.net/~iklam/8009575/symbol_refcount_002/
>>>
>>> Bug: Reduce Symbol::_refcount from 4 bytes to 2 bytes
>>>
>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8009575
>>>      https://jbs.oracle.com/bugs/browse/JDK-8009575
>>>
>>> Summary of fix:
>>>
>>>      As noted in the bug report, the main problem is _refcount needs 
>>> to be
>>>      atomically incremented, but the smallest unit that 
>>> Atomic::incr() had
>>>      supported until now was 32-bit int.
>>>
>>>      With Coleen's help, I have created a platform-independent version
>>>      of Atomic::incr(short*) that's based on Atomic::add(int*, n).
>>>
>>>      Essentially, Atomic::incr(short*) can be implemented as
>>>      Atomic::add(int*, 0x10000), as long as the short occupies the most
>>>      significant 16 bits of the int. I added the macro 
>>> ATOMIC_SHORT_PAIR
>>>      to ensure the proper alignment.
>>
>> I preferred the original version that constrained this to 
>> symbol.hpp/cpp. We don't really support Atomic jshort operations in a 
>> general sense - we only have inc/dec and even then this only applies 
>> when you have the pair of shorts with only one needing atomic access! 
>> So I would have preferred not to see this exposed in the Atomic class.
>
> Hi David,
>
> I had opposite opinion and urged Ioi to hide the atomic short 
> operations in the atomic class.   It might be of general use and he 
> implemented it with the minimal implementation details to distract the 
> Symbol class.  I have reviewed and like this change. It is really 
> clean and doesn't expose endianness where it isn't interesting.

I'll have to concur that I think the ENDIAN-ness is encapsulated in
the right place in this version... Now if I can only come up with
something as clean when I get back to padding in monitors/locks...

Dan


>
>>
>> Minor comment: are the friend declarations needed for both Symbolbase 
>> and Symbol?
>
> Looks like a lot of friendliness.
>
> thanks,
> Coleen
>>
>> Thanks,
>> David
>>
>>>      Also Symbol::size(int) is fixed to calculate the correct space 
>>> needed
>>>      for a Symbol. (Same as in the previos webrev --
>>>      see
>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2013-March/006385.html) 
>>>
>>>
>>>
>>> Tests:
>>>
>>>      JPRT
>>>      UTE:
>>>         - vm.tmtools.testlist nsk.sajdi.testlist vm.runtime.testlist
>>>           vm.quick.testlist vm.parallel_class_loading.testlist
>>>         - Serviceability Agent tested are included in these lists.
>>>      MedRec
>>>
>>> Thanks
>>> - Ioi
>>>
>