Request for Reviews 7173918&7173919: Small follow-ons to alternative hashing (original) (raw)

David Holmes david.holmes at oracle.com
Mon Jun 4 23:04:53 UTC 2012

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

More information about the core-libs-dev mailing list