[9] Review request for 8140503: JavaFX "Pair" Hash Code Collisions

[Alexander Kouznetsov]

I've missed the most important one: "here is a trivial collision of two 
hash codes which should be different."

Best regards,
Alexander Kouznetsov
(408) 276-0387

On 3 ноя 2015 13:43, Alexander Kouznetsov wrote:
> Moreover, the following two sentences:
>
> "However, this is an incorrect way to compute a hash code of two values."
> "This can lead to hard-to-find bugs anywhere that instances of Pair 
> are used in a data structure like a HashSet or HashTable."
>
> seem to indicate misunderstanding of what hashcode is and how it is to 
> be used.
>
> Best regards,
> Alexander Kouznetsov
> (408) 276-0387
>
> On 3 ноя 2015 13:42, Alexander Kouznetsov wrote:
>> After the fix, you should expect another incident report of
>>
>> Objects.hash(1, 0) == Objects.hash(0, 31)
>>
>> always true :-)
>>
>> I'd rather file another bug on key == null causing NPE and closing 
>> this one as incomplete or not an issue.
>>
>> Best regards,
>> Alexander Kouznetsov
>> (408) 276-0387
>>
>> On 3 ноя 2015 12:07, Vadim Pakhnushev wrote:
>>> Hmm, yeah, the actual difference is in the prime number only (that 
>>> is changing the algorithm only doesn't improve anything), so the 
>>> only remaining reason to fix this is that Objects.hash guards 
>>> against null values (and I forgot to mention it in the review).
>>> The key in Pair could actually be null and in this case hashCode 
>>> will throw NPE.
>>>
>>> Vadim
>>>
>>> On 03.11.2015 23:01, Vadim Pakhnushev wrote:
>>>> Well, not exactly... Previously it was 13*hash(a) + hash(b) and now 
>>>> it's 31*(31 + hash(a)) + hash(b).
>>>> And apparently it improves the quality somehow. I did a test with 
>>>> 100^4 combinations and collision probability dropped by the factor 
>>>> of 3 from 0.065% to 0.022%.
>>>> Not really impressive, but still, and it uses well-defined utility 
>>>> method.
>>>> Yeah, I know it's not really a bug since you don't want to rely on 
>>>> the hashCode at all...
>>>>
>>>> Thanks,
>>>> Vadim
>>>>
>>>> On 03.11.2015 22:35, Jim Graham wrote:
>>>>> All this does is change the prime constant used to produce the 
>>>>> hash value.
>>>>>
>>>>> Objects.hash(a, b) uses 31*hash(a) + hash(b) instead of the 
>>>>> 13*hash(a) + hash(b) that the embedded implementation uses.
>>>>>
>>>>> I don't really think this is a bug.  The fact that Integer objects 
>>>>> make it easy to reverse engineer and compute collisions of any 
>>>>> reasonable hash combination computation don't mean that the 
>>>>> technique has a bug, it just means that the submitter can read the 
>>>>> code and think of a counter-example.
>>>>>
>>>>> If there are practical problems being caused for some particular 
>>>>> and popular use case by the use of this particular constant "13", 
>>>>> then we need to understand those issues and come up with a more 
>>>>> comprehensive solution than to simply hand off to another 
>>>>> mechanism which uses the same procedure with a different prime 
>>>>> constant...
>>>>>
>>>>>             ...jim
>>>>>
>>>>> On 11/3/15 3:06 AM, Vadim Pakhnushev wrote:
>>>>>> Hi Chien,
>>>>>>
>>>>>> Could you please review the fix:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8140503
>>>>>> http://cr.openjdk.java.net/~vadim/8140503/webrev.00/
>>>>>>
>>>>>> Thanks,
>>>>>> Vadim
>>>>
>>>
>>
>