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
