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
