Request for Reviews 7173918&7173919: Small follow-ons to alternative hashing

[David Holmes]

Ulf,

As I understand Mike's email, the JDK7 code is not being modified (at 
this stage at least) to apply the optimizations that have been applied 
to the JDK8 code. The JDK 7 changes are only to set the default value to 
512 (plus that one final field change)

David

On 4/06/2012 10:39 PM, Ulf Zibis wrote:
> Am 04.06.2012 08:08, schrieb Mike Duigou:
>> Two small issues related to the larger alternative string hashing
>> changes (CR#7126277) from last week. One issue is for JDK 7 and the
>> other is for JDK 8. Both are quite small.
>>
>> For the JDK 7 issue, as documented in the review request, the default
>> threshold for the alternative hashing in that patch was set to the
>> minimum to unconditionally enable the feature. This was done to make
>> compatibility testing easier. Developers are strongly encouraged to
>> test their applications with the 7u6b13 build to ensure that their
>> applications do not later encounter problems. For the release version
>> of 7u6 the default threshold will be set to 512. This patch sets that
>> default and marks one field final.
>>
>> http://cr.openjdk.java.net/~mduigou/7173918/0/webrev/
> *** WeakHashMap
>
> 353 int hash(Object k) {
> 354
> 355 int h;
> 356 if (useAltHashing) {
> 357 h = hashSeed;
> 358 if (k instanceof String) {
> 359 return h ^ sun.misc.Hashing.stringHash32((String) k);
> 360 } else {
> 361 h ^= k.hashCode();
> 362 }
> 363 } else {
> 364 h = k.hashCode();
> 365 }
>
> ==> shorter form + better chance for inlining + less less jumps, which
> are always kinda expensive:
>
> 353 int hash(Object k) {
> 354
> 355 int h;
> 356 if (useAltHashing) {
> 357 h = hashSeed;
> 358 if (k instanceof String) {
> 359 return h ^ sun.misc.Hashing.stringHash32((String)k);
> ==> Remove the space after the cast.
> 360 }
> 361
> 362 h ^= k.hashCode();
>
> *** HashMap
> doesn't use "(h = hashSeed) ^ ..." for the String case, why?
>
> *** Hashtable
> has completely different design, why? :
> - doesn't use instanceof operator
> - doesn't use "(h = hashSeed) ^ ..." for general objects
>
> ==> You again have inserted a spaces after casts.
>
>>
>> The JDK8 issue is number of small performance enhancements suggested
>> by Ulf Zibis and Remi Forax. These changes are being considered now
>> while this issue remains fresh in the maintainer and reviewer's minds.
>>
>> http://cr.openjdk.java.net/~mduigou/7173919/0/webrev/
>
> 291 if (k instanceof String) {
> 292 return ((String) k).hash32();
> ==> You again have inserted a space after the cast.
> 293 }
> ==> Please add a blank line here as in the other map classes. Otherwise
> it would look, that the following line has some context to the before line.
> ==> Additionally I would add a comment here, why the legacy behaviour on
> general objects is changed in that way.
> ==> Maybe it would be more readable/logical to move this line just after
> line 298.
> 294 int h = hashSeed ^ k.hashCode();
> 295
> 296 // This function ensures that hashCodes that differ only by
> 297 // constant multiples at each bit position have a bounded
> 298 // number of collisions (approximately 8 at default load factor).
> 299 h ^= (h >>> 20) ^ (h >>> 12);
> 300 return h ^ (h >>> 7) ^ (h >>> 4);
>
> Anyway I still would prefer to move the hash(Object) method to
> AbstractHashMap.
> Additionally while being here, we could review, why ConcurrentHashMap
> has a different algorithm.
>
> -Ulf
>
>