Okay, you convinced me. :) Here's another webrev [1], updated to make use of Objects.hashCode() in the fix (as well as in the testcase).
Can I now interest a Reviewer in endorsing this fix ? Thanks, Neil [1] http://cr.openjdk.java.net/~ngmr/8000955/webrev.03/ On Tue, 2012-10-16 at 09:13 -0700, Mike Duigou wrote: > On Oct 15 2012, at 20:53 , Neil Richards wrote: > > > Hi Mike, > > Thanks for the swift appraisal. > > > > Good suggestion to expand the test to cover other Map implementations - > > I'd toyed with the idea, but hadn't spotted Collisions.java as a > > template to follow. > > > > I've posted an new webrev [2] with the updated (and moved) testcase, > > which also makes use of Objects.hashCode() . > > This test looks suspiciously like it anticipates TestNG DataProviders. :-) > Soon. > > > Might there be a performance impact of using Objects.hashCode() in the > > fix itself, rather than having the logic inline? > > In other cases we found that the JIT always inlined the method and there was > no performance penalty. Only if the hashCode method itself got too big did > the method invocation matter. > > > On the surface, it looks like it would be an extra method call, but > > maybe the JIT would iron that out? > > That is the expectation. Hotspot at least will inline this method and there > has been talk of making Objects.hashCode() and a few other methods into > intrinsics which would further improve inlining. > > > (I fret about this as (unnecessary) impacts in Hashtable performance > > tend to widely felt). > > Understood and agreed. > > > > > I notice that the other Map.Entry implementations (that I've looked at > > so far) have this logic inlined, which is one reason why I went that > > route. > > I believe that this is primarily historical from before Objects.hashCode > existed. I have started to convert instances of this logic but it's too small > of an issue (and I am too busy) to attempt to convert existing instances as > an independent task. > > > Thanks for raising 8000956 for the Java 7 backport. > > I guess this means you think the fix should get back there fairly > > swiftly. > > We should try to get it pushed in to 7 ASAP once we are done in 8. > > > Presumably 8000956 is linked to 8000955, so I would take the same > > change, with the same commit comment - crucially using the same, > > original bug id, 8000955 - back onto jdk7u-dev ? > > Correct. > > > (This is what I've done for other items I've helped in taking back to > > jdk7u-dev. Using the same bug id makes it easier to track changes that > > have been applied across different streams). > > > > I'm guessing I also need to raise this on the jdk7u-dev mailing list and > > get the nod (from Sean Coffey) to put it back there ? > > Yes. > > > Prior to this, I think I need a blessing for this change from a jdk8 > > "Reviewer" to push the change to jdk8/tl. > > > > > > Regards, > > Neil > > > > [2] http://cr.openjdk.java.net/~ngmr/8000955/webrev.02 > > > > On Mon, 2012-10-15 at 18:48 -0700, Mike Duigou wrote: > >> Good find Neil. > >> > >> I have opened JDK-8000955 for this issue along with JDK-8000956 for the > >> Java 7 backport. > >> > >> The code could be simplified by using Objects.hashCode() > >> > >> ie. > >> > >> return Objects.hashCode(key) ^ Objects.hashCode(value); > >> > >> Any objection to extending the test similar to Collisions.java to test > >> other Map classes? > >> > >> Mike > >> > >> On Oct 15 2012, at 18:29 , Neil Richards wrote: > >> > >>> Hi, > >>> I notice that the behaviour of java.util.Hashtable.Entry.hashCode() no > >>> longer conforms to the defined behaviour (in the Java API Javadoc [1]) > >>> for java.util.Map.Entry.hashCode() implementations. > >>> > >>> The code in Hashtable.Entry.hashCode() assumes that the value in > >>> Hashtable.Entry.hash will always be the same as that for > >>> Hashtable.Entry.getKey().hashCode() . > >>> > >>> However, since Java bug 7126277 (Alternative String hashing > >>> implementation), the use of Hashtable.hashSeed, a randomizing factor, > >>> has been introduced into the calculation of Hashtable.hash(). > >>> > >>> It is the result from Hashtable.hash() which ends up stored in the > >>> Hashtable.Entry.hash field. > >>> > >>> So the assumption made in Hashtable.Entry.hashCode() is no longer valid, > >>> and the code needs to be corrected, so that it once more complies with > >>> the Java API defined behaviour. > >>> > >>> > >>> I have created a webrev [2] with my suggested fix for this problem, > >>> together with a simple testcase to demonstrate it. > >>> > >>> (I've also checked the other Map implementations modified by 7126277, > >>> and verified that the others still conform to the Java API in this > >>> regard). > >>> > >>> Please review the problem and suggested fix, and let me know your > >>> thoughts. > >>> > >>> I am not aware of an existing Java bug for this issue. > >>> Provided you agree with my analysis, could one be raised to allow the > >>> fix to be committed? > >>> > >>> Thanks, > >>> Neil > >>> > >>> [1] > >>> http://docs.oracle.com/javase/7/docs/api/java/util/Map.Entry.html#hashCode%28%29 > >>> [2] > >>> http://cr.openjdk.java.net/~ngmr/hashtable-entry-hashcode-fix/webrev.00 > >>> > >>> -- > >>> Unless stated above: > >>> IBM email: neil_richards at uk.ibm.com > >>> IBM United Kingdom Limited - Registered in England and Wales with number > >>> 741598. > >>> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU > >>> > >> > > > > > > -- > > Unless stated above: > > IBM email: neil_richards at uk.ibm.com > > IBM United Kingdom Limited - Registered in England and Wales with number > > 741598. > > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU > > > -- Unless stated above: IBM email: neil_richards at uk.ibm.com IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
