[7u6] Request for approval / patch for review : 7180240 : Disable alternative string hashing by default

[Stuart Marks]

Hi Mike,

The changes themselves seem simple enough.

I have a few other comments though. I didn't see them raised in the earlier 
reviews, though of course I could have missed something. Please use your 
judgment in deciding what changes you make in response.

There's an odd asymmetry between CHM and the other hashmaps in that unlike the 
others, CHM lacks the ALTERNATE_HASHING_THRESHOLD_DEFAULT constant. Instead the 
default is inlined. This is not a huge deal but the other hashmaps have some 
useful explanatory doc for ALTERNATE_HASHING_THRESHOLD_DEFAULT and this is 
missing from CHM. The inlining of Integer.MAX_VALUE in the computation of the 
threshold is pretty opaque, and to someone who hasn't followed this whole saga 
it's not very apparent that this disables alternative hashing by default (for CHM).

At the very least it's worth a small comment at the point of the change in CHM 
(line 199) indicating it's disabled by default. It might be worth refactoring 
this to use ALTERNATE_HASHING_THRESHOLD_DEFAULT like the other hashmaps.

Another point is that the threshold is used differently by CHM than by the 
other hashmaps. For CHM alternative hashing is either always on or always off, 
whereas for the other hashmaps the alternative hashing is enabled and disabled 
as the map grows and shrinks. It could be that this was intentional, but such 
an inconsistency from the other maps should be documented -- particularly since 
it's controllable from a system property. Another thing to consider is that 
since CHM is controlled differently, it might want to use a different property.

The docs for ALTERNATE_HASHING_THRESHOLD_DEFAULT in the non-CHM hashmaps 
mention the property java.util.althashing.threshold, but the code (for all the 
maps) actually checks for jdk.map.althashing.threshold. Which is correct?

Also, this doc in Hashtable.java recommends using -1 to disable alternative 
hashing, whereas HashMap.java and WeakHashMap.java recommend using 2147483648 
(Integer.MAX_VALUE). The correct value of Integer.MAX_VALUE is 2147483647, but 
it seems easier to recommend disabling via -1 instead.

<bikeshed>
Various docs and variable names seem to use "alternate" vs "alternative" 
interchangeably. I prefer "alternative".
</bikeshed>

s'marks




On 6/27/12 2:34 PM, Mike Duigou wrote:
> Hello all;
>
> Following testing and feedback the alternative hashing string keys in hash maps which was introduced in CR#7126277 is going to be disabled by default in 7u6. Developers can still enable the alternative hashing but by default it will be disabled. More time is required for developers to test their applications and correct improper usages before this feature can be enabled as the default behaviour. The alternative hashing of String keys feature remains the default for Java 8 and may become the default for Java 7 in a future release.
>
> Developers are strongly encouraged test their applications by enabling the alternative string hashing feature before it does become the default behaviour. The alternative Sring hashing feature is enabled by setting the system property, jdk.map.althashing.threshold to a value smaller than the capacity of the maps to be tested. The future default is likely to be "512" which will have the effect of enabling alternative hashing of string keys for all maps who's capacity is larger than 511 entries. Small maps only encounter limited impact from collisions and the higher threshold also masks incidental dependence upon iteration order that may be present in those maps. For the most rigorous testing, set the jdk.map.althashing.threshold property to "1" which will force all maps to use alternative string hashing.
>
> The current patch for review:
>
> http://cr.openjdk.java.net/~mduigou/7180240/0/webrev/
>
> This change is not a back port because jdk8 uses a different implementation and is unaffected.
>
> When approved and reviewed I intend to push it to
>   ssh://hg.openjdk.java.net/jdk7u/jdk7u-dev-gate/jdk
>
> Regards,
>
> Mike