RFR(S): 8087143: Reduce Symbol::_identity_hash to 2 bytes

[Coleen Phillimore]

Yumin,

If the identity hash isn't unique enough you get a system dictionary 
warning like this, which doesn't stop the JVM.   Could you check your 
test logs to see if this warning is output in any of them (jtreg keeps 
logs around, just checking the jtreg test results should be sufficient, 
I think).

Log : HotSpot(TM) Server VM warning: Performance bug: SystemDictionary 
lookup_count=6918 lookup_length=3907 average=0.564759 load=0.266601

I think with including _body[0] in the function you may make the hash 
random enough.   So this was one of my concerns with this change.

In 
http://cr.openjdk.java.net/~minqi/8087143/webrev01/src/share/vm/runtime/java.cpp.udiff.html

Are you sure you want all of the SymbolTable dump?  Maybe it should be 
under PrintSymbolTableSizeStatistics instead?

In 
http://cr.openjdk.java.net/~minqi/8087143/webrev01/src/share/vm/oops/symbol.hpp.udiff.html

-// We separate the fields in SymbolBase from Symbol::_body so that
-// Symbol::size(int) can correctly calculate the space needed.

I'm not sure what that comment meant.  With SymbolBase gone (which I 
think looks nicer), can you still correctly calculate the space needed?

Other than these things, this change looks good.  Nice to find some 
space savings with something so frequent as symbols.

Thanks,
Coleen

On 6/18/15 8:40 PM, Yumin Qi wrote:
> Ioi,
>
>   Thanks for review, I will add that (for print out purpose, not in 
> the final change?)
>
> Yumin
>
> On 6/18/2015 3:34 PM, ioi.lam at oracle.com wrote:
>> Hi Yumin,
>>
>> I wonder why the average symbol size didn't change. They are both 
>> about 34.25 bytes long.
>>
>> When I tested the change, -Xshare:dump -XX:+PrintSharedSpaces show 
>> about 1.3 bytes reduction per symbol.
>>
>> Also, could you add some code to print out the statistics of the 
>> system dictionary to show if there's any change in the hashtable 
>> balance?
>>
>> Thanks
>>
>>> On Jun 18, 2015, at 1:46 PM, Yumin Qi <yumin.qi at oracle.com> wrote:
>>>
>>> Please review the small change for
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8087143
>>> webrev: http://cr.openjdk.java.net/~minqi/8087143/webrev01/
>>>
>>> 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.
>>>
>>> This is a minor change, also eliminates SymbolBase which seems 
>>> redundant.  Also add print out SymbolTable statistics under debug 
>>> flag PrintSystemDictionaryAtExit.
>>>
>>> Tests: JPRT, Specjbb2005.
>>>
>>> The SymbolTable statistics for specjbb2005:
>>>
>>> Before Change:
>>> SymbolTable statistics:
>>> Number of buckets       :     20011 =    320176 bytes, avg 16.000
>>> Number of entries       :     49914 =   1597248 bytes, avg 32.000
>>> Number of literals      :     49914 =   1709688 bytes, avg 34.253
>>> Total footprint         :           =   3627112 bytes
>>> Average bucket size     :     2.494
>>> Variance of bucket size :     2.483
>>> Std. dev. of bucket size:     1.576
>>> Maximum bucket size     :        10
>>>
>>> After Change:
>>>
>>> SymbolTable statistics:
>>> Number of buckets       :     20011 =    320176 bytes, avg 16.000
>>> Number of entries       :     49911 =   1597152 bytes, avg 32.000
>>> Number of literals      :     49911 =   1709544 bytes, avg 34.252
>>> Total footprint         :           =   3626872 bytes
>>> Average bucket size     :     2.494
>>> Variance of bucket size :     2.483
>>> Std. dev. of bucket size:     1.576
>>> Maximum bucket size     :        10
>>>
>>> There is no big change for the hashtable balance.
>>>
>>>
>>> Thanks
>>> Yumin
>