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

[Daniel D. Daugherty]

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

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


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
>