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

[David Holmes]

Hi Ioi,

First your frames view in the webrev continues to be broken (missing 
file) :(

On 27/03/2013 7:03 AM, Ioi Lam wrote:
> Please review:
>
> http://cr.openjdk.java.net/~iklam/8009575/symbol_refcount_001/
> <http://cr.openjdk.java.net/%7Eiklam/8009575/symbol_refcount_001/>

Looking at generateJvmOffsets.cpp:

I don't understand this part of the code so can't comment.

Looking at vmStructs.cpp:

I don't see any SA changes so I assume these values never actually get 
accessed ?? How do you verify this part of the change?

> 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:
>
> (1) Symbol::_refcount has been changed to a short.
>
>      Note that _refcount needs to be incremented atomically using
>      Atom::add, which requires a 32-bit word in memory. So I glue the
>      _refcount and _length fields together using union and ENDIAN
>      tricks, which is kind of ugly but faster.
>
>      The prettier alternative would be to use shift and masking to
>      access the _refcount and _length components, but performance
>      would be slightly slower.
>
>      If anyone has a more elegant solution, please let me know.

Not sure why shift and mask would be slower as the compiler will have to 
generate shifts and masks anyway ??

>      #------------------------------------------------------
>        union {
>          volatile int   _refcount_and_length;
>          struct {
>      #ifdef VM_LITTLE_ENDIAN
>            unsigned short _length; // number of UTF8 characters in the
> symbol
>            volatile short _refcount;
>      #else
>            volatile short _refcount;
>            unsigned short _length; // number of UTF8 characters in the
> symbol
>      #endif
>          } s;
>        } u;
>
>      void Symbol::increment_refcount() {
>        if (u.s._refcount >= 0) {
>          // Adding 0x10000 to _refcount_and_length is equivalent to
> adding 0x1 to
>          // _refcount (w.r.t overflow/underflow).
>          (void)Atomic::add(0x10000, &u._refcount_and_length);
>        ....
>      }
> #------------------------------------------------------

As per side email a key enabler here is that _length is immutable once set.

I was going to suggest that the fields be swapped so that we just 
add/subtract 1, but I see now that the order is necessary to ensure the 
overflow detection works without modifying _length.

> (2) Symbol::size(int) was wasting one byte per Symbol instance. I fixed
> it by
>      splitting part of the fields from Symbol into SymbolBase.

I don't understand this part. What were we wasting? And given alignment 
constraints did we in fact waste it?

> Footprint result:
>
>      The theoretical reduction is 3 bytes per symbol -- 2 bytes
>      from _refcount, and 1 byte from the fixed Symbol::size(int)
>      function.
>      In actual measurement, CDS image (about 3000 classes, with about
>      50000 symbols) is reduced by about 140KB, or about 2.8 bytes per
>      Symbol, or about 0.6% of the whole CDS image.

I'm not sure that makes sense. I would expect you to save 0 bytes or 4 
bytes depending on alignment (assuming the Symbol is 32-bit aligned to 
start with).


Thanks,
David

>
> Testing:
>
>      * UTE (vm.runtime.testlist, vm.quick.testlist
> vm.parallel_class_loading.testlist)
>
> Thanks,
> Ioi
>