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
