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

[David Holmes]

Hi Mike,

You seem to have forgotten to now disable alt hashing in Hashtable.java 
- the default is back to 512.

Otherwise, apart from the <p/> in CHM I don't see any problems.

Thanks,
David

On 29/06/2012 2:31 AM, Mike Duigou wrote:
> I have updated the webrev to include feedback from reviewers and improve the documentation.
>
> http://cr.openjdk.java.net/~mduigou/7180240/1/webrev/
>
> The only substantive changes are to rename several fields to be more consistent about naming. ie. use "alternative" rather than "alternate".
>
> I also added the missing spaces after "if" in a few cases.
>
> Mike
>
>
> On Jun 27 2012, at 15:46 , Stuart Marks wrote:
>
>> Oh, one more thing...
>>
>> If there is an illegal negative value or a syntactically invalid value given to the property, this will throw an Error from the Holder class, which will probably cause the VM to shut down fairly quickly since no hashmap instances can be created. I guess this it's OK to fail fast, as opposed to silently ignoring the value, but I wanted to check to make sure this was the intent.
>>
>> s'marks
>>
>> On 6/27/12 3:40 PM, Stuart Marks wrote:
>>> 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
>