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

[Ulf Zibis]

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