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

[Ioi Lam]

On 06/21/2013 11:15 AM, Daniel D. Daugherty wrote:
> On 6/20/13 12:20 PM, 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/
>
> Nicely done! Thumbs up...
>
> src/share/vm/oops/symbol.hpp
>     line 112: ATOMIC_SHORT_PAIR(
>         Now this has me intrigued...
>
>     line 132: size_t sz = heap_word_size(sizeof(SymbolBase) + (length 
> > 0 ? length : 0));
>         Assuming that 'length' can't be < 0, this is more clear:
>
>         assert(length >= 0, "sanity check");
>         size_t sz = heap_word_size(sizeof(SymbolBase) + length);
>
> src/share/vm/oops/symbol.cpp
>     line 35: Symbol::Symbol(const u1* name, int length, int refcount) {
>     line 36:   _refcount = refcount;
>     line 37:   _length = length;
>         Any particular reason for changing init styles? Otherwise, this
>         is just noise in the review... I'm OK if you keep the newer
>         init style (it's actually my preferred style)...
>
I was just too lazy to add a SymbolBase constructor. The "orthodox C++ 
style" would be:

   Symbol::Symbol(const u1* name, int length, int refcount) : 
SymbolBase(length, refcount) {...}

But I don't like this either -- (you can swap length and refcount by 
mistake it's hard to tell the error).

So I'll leave the code as it now :-)

> src/share/vm/runtime/atomic.hpp
>     I like the encapsulation of ENDIAN-ness in the the ATOMIC_SHORT_PAIR!
>
> src/share/vm/runtime/atomic.cpp
>     I was trying to figure out a way to add verification that the "other"
>     short is not affected, but anything I can come up with would be 
> "racey".
>
> src/share/vm/runtime/vmStructs.cpp
>     line 382: volatile_nonstatic_field(Symbol, _refcount, short)
>         Sorry my vmStructs-foo is very rusty, but should this be
>         "SymbolBase" instead of "Symbol"?
>

vmStructs.cpp has macros like this:

#define CHECK_NONSTATIC_VM_STRUCT_ENTRY(typeName, fieldName, 
type)                 \
  {typeName *dummyObj = NULL; type* dummy = 
&dummyObj->fieldName;                   \
   assert(offset_of(typeName, fieldName) < sizeof(typeName), "Illegal 
nonstatic struct entry, field offset too large"); }

Because VMStructs is a friend class of both SymbolBase and Symbol, it 
can access either of the following fields (they are the same field that 
is inherited by Symbol from SymbolBase)

     &(SymbolBase::_refcount)
or
     &(Symbol::_refcount)

Since SymbolBaseis really an internal type, I think it's better to use 
Symbol in vmStructs.cpp

Thanks
- Ioi

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