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

[Yumin Qi]

Thanks for your suggestion, that is good to cover the concern. I will 
factor a test case for it.

Yumin
On 7/27/2015 9:29 PM, Ioi Lam wrote:
> 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
>