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
