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

[Coleen Phillimore]

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.

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