Review Request CR#7118743 : Alternative Hashing for String with Hash-based Maps

[Mike Duigou]

On May 30 2012, at 17:59 , Rémi Forax wrote:

> On 05/30/2012 06:23 PM, Mike Duigou wrote:
>> On May 30 2012, at 07:38 , Rémi Forax wrote:
>> 
>>> On 05/30/2012 01:05 AM, Mike Duigou wrote:
>>>> Another round of updates for Java 7 [1] and Java 8 [2] implementations. This revision incorporates Remi's suggestions and some feedback from Doug Lea regarding applying the per-instance seed to the result of String.hash32()
>>>> 
>>>> [1] althashing "7" webrev : http://cr.openjdk.java.net/~mduigou/althashing7/10/webrev/
>>>> [2] althashing "8" webrev : http://cr.openjdk.java.net/~mduigou/althashing8/10/webrev/
>>>> 
>>>> Barring any emergencies this will be integrated to both Java 7 and Java 8 on Wednesday May 30th, 2012.
>>>> 
>>>> Thanks to all who provided feedback!
>>>> 
>>>> Mike
>>> Hi Mike,
>>> I've still trouble to understand why you want to expose something
>>> which is not used.
>>> 
>>> We know that the implementation of String.hashCode()
>>> is broken but we can't change it because the implementation
>>> is part of the spec.
>>> We can't do an instanceof check on an interface in the implementations
>>> of Map because the VM implementations (not only Hotspot by the way)
>>> are not able to transform it to a quick class check.
>>> String.hash32 is not a polymorphic method but just a method
>>> used to fix the fact that we can't change the implementation
>>> of String.hashCode() so there is no need for a interface like Hashable32.
>>> 
>>> as a proof, as Ulf said, String even doesn't implement Hashable32.
>> 
>> It's a mistake that String doesn't implement Hashable32 in the java 8 patch. That line somehow got lost with multiple revisions. (most likely due to constant merge conflicts with the String offset/count patch) I will restore it.
>> 
>> The current proposal for Java 8:
>> 
>> - A new interface Hashable32 is introduced.
>> - Hashable32 provides a method hash32()
>> - String implements Hashable32 and hash32() method
>> - HashMap et al recognize String and invoke hash32() rather than hashCode()
>> - ** End of current implementation **
>> - When the mainline javac supports extensions methods the implementation will be extended.
>> - Add extension method to Hashable32.hash32() that calls hashCode()
>> - Object implements Hashable32  [controversial idea]
>> - HashMap et al unconditionally call hash32(). Other JDK code may also switch to call hash32() instead of hashCode()
> this item is controversial too because HashMap specifies that it use hashCode() not hash32.
> 
> Anyway, Hashable32 is not needed for this patch and can be introduced later without any problem.
> So I prefer to postpone the introduction of Hashable32 until we will be able to see if it's the a good idea or not

Fair enough. I am eager to get this patch in so I will remove it if that is the only remaining objection.

Mike