RFR: 8130115: REDO - Reduce Symbol::_identity_hash to 2 bytes

[Ioi Lam]

Hi Yumin,

The C code changes look good to me.

I am a little concerned about the Java code's calculation of identityHash:

Java version:
   86   public int identityHash() {
   87     long addr_value = getAddress().asLongValue();
   88     int  addr_bits = (int)(addr_value >> 
(VM.getVM().getLogMinObjAlignmentInBytes() + 3));
   89     int  length = (int)getLength();
   90     int  byte0 = getByteAt(0);
   91     int  byte1 = getByteAt(1);
   92     int  id_hash = (int)(0xffff & idHash.getValue(this.addr));
   93     return id_hash |
   94            ((addr_bits ^ (length << 8) ^ ((byte0 << 8) | byte1)) 
<< 16);
   95   }

C version:
  148   unsigned identity_hash() {
  149     unsigned addr_bits = (unsigned)((uintptr_t)this >> 
(LogMinObjAlignmentInBytes + 3));
  150     return (unsigned)_identity_hash |
  151            ((addr_bits ^ (_length << 8) ^ (( _body[0] << 8) | 
_body[1])) << 16);
  152   }

The main problem is to correctly emulate the C unsigned operations in 
the Java code. I've eyeballed the code and it seems correct, but I am 
wondering if you have actually tested and verified that the Java version 
indeed returns the same value as the C code? A unit test case would be good:

  * Add a new test in hotspot/agent/test
  * Get a few Symbols (e.g., call
    sun.jvm.hotspot.runtime.VM.getSymbolTable and iterate over the first
    1000 Symbols)
  * For each Symbol, call its Symbol.identityHash() method
  * Add a new whitebox API to return the C version of the
    identity_hash() value
  * Check if the C value is the same as the Java value

Please run the test on all platforms (both 32-bit and 64-bit, and all OSes).

Thanks
- Ioi


On 7/15/15 10:37 AM, Yumin Qi wrote:
> Hi,
>
>   This is redo for bug 8087143, in that push, it caused failure on 
> Serviceability Agent failed to get type for "_identity_hash": 
> mistakenly used JShortField for it, but in fact it still is 
> CIntegerField. In this change, besides of the original change in 
> hotspot/src, I add code to calculate identity_hash in hotspot/agent 
> based on the changed in hotspot.
>
> Old webrev for 8087143:
> bug: https://bugs.openjdk.java.net/browse/JDK-8087143
> webrev: http://cr.openjdk.java.net/~minqi/8087143/webrev03/
>
> Summary: _identity_hash is an integer in Symbol (SymbolBase), it is 
> used to compute hash bucket index by modulus division of table size. 
> Currently in hotspot, no table size is more than 65535 so we can use 
> short instead.  For case with table size over 65535 we can use the 
> first two bytes of symbol data to be as the upper 16 bits for the 
> calculation but rare cases.
>
> New webrev for 8130115:
> bug: https://bugs.openjdk.java.net/browse/JDK-8130115
> webrev: http://cr.openjdk.java.net/~minqi/8130115/webrev01/
>
>
> Tests: JPRT, SA manual tests, -atk quick, jtreg hotspot/runtime
> Also internal large application used for hashtable data analysis --- 
> the No. of loaded classes is big(over 19K), and tested with different 
> bucket sizes including over 65535 to see the new algorithm for 
> identity_hash calculation, result shows the consistency before and 
> after the fix.
>
> Thanks
> Yumin