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

[Coleen Phillimore]

Hi,  This is okay, one comment below.

On 6/19/15 12:21 PM, Yumin Qi wrote:
> Coleen,
>
>   Thanks.
>
> On 6/19/2015 5:14 AM, Coleen Phillimore wrote:
>>
>> 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.
>>
> I will check that.
>> 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?
>>
> I will check and make change based on the output of statistics, maybe 
> it is too much to print out all SymbolTable.

It's way more than anyone needs normally.   Maybe only print it if one 
adds +Verbose to PrintStringTableSizeStatistics and when we do logging, 
it'll be -Xlog:symboltable=trace or a lower logging level than info.

Thanks,
Coleen
>> 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?
>>
> Yes, now the _body changed from 1 byte array to 2 bytes array.
> The original calculation is not correct I think:  if symbol length > 
> 0, we allocate *_ONE_* extra byte for it:
> -    size_t sz = heap_word_size(sizeof(SymbolBase) + (length > 0 ? length : 0));
> +    size_t sz = heap_word_size(sizeof(Symbol) + (length > 2 ? length - 2 : 0));
>
>> 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
>>>
>>
>