RFR: 8000955: Hashtable.Entry.hashCode() no longer conforms to Map.Entry.hashCode()

Tue Oct 16 23:12:17 UTC 2012

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