RFR: 8033792: AltHashing used jint for imprecise bit shifting
Yumin Qi
yumin.qi at oracle.com
Mon Feb 10 12:41:40 PST 2014
Thanks for Coleen and Calvin for the review!
Yumin
On 2/10/2014 11:34 AM, Coleen Phillimore wrote:
>
> This looks really good. Thank you!
> Coleen
>
> On 2/8/14 12:08 AM, Yumin Qi wrote:
>> Hi, David and all
>>
>> New webrev:
>> http://cr.openjdk.java.net/~minqi/8033792/webrev01
>>
>> Changed _seed to be defined as 'juint' and changed related uses.
>> This can avoid loss of precision for conversion between 'jint' and
>> 'juint'.
>> Tested with -XX:+ExecuteInternalVMTests and JPRT.
>>
>> InstanceKlass::host_klass:
>> Add assert for the path which has to contain a none NULL valid
>> Klass* (we did not check for valid Klass*, only check if it is not a
>> NULL here) in debug mode.
>>
>>
>> Thanks
>> Yumin
>>
>>
>> On 2/7/2014 9:54 AM, Yumin Qi wrote:
>>>
>>> On 2/6/2014 11:47 PM, David Holmes wrote:
>>>> Hi Yumin,
>>>>
>>>> On 7/02/2014 2:14 PM, Yumin Qi wrote:
>>>>> Hi,
>>>>>
>>>>> Please review the change for 8033792.
>>>>>
>>>>> Summary: AltHashing uses 'jint' type in the way as 'unsigned
>>>>> int' in
>>>>> bit shifting, which is imprecise. This could lead loss of precision
>>>>> when converted between jint and 'unsigned int' during bit
>>>>> operation. Fix
>>>>> by changing operation variable type from 'jint' to 'juint', before
>>>>> return, cast it into type 'jint'.
>>>>>
>>>>> http://cr.openjdk.java.net/~minqi/8033792/webrev00/
>>>>
>>>> I don't see how casting to unsigned int can lead to a loss of
>>>> precision here. Further you now assign h1 from a jint and you pass
>>>> it as a jint parameter to the Integer_rotate functions which would
>>>> seem to me to have more opportunity for conversion issues. Either
>>>> way this code is completely confused about the type of arithmetic
>>>> it is trying to do (and I would think a hash should be unsigned to
>>>> begin with ??).
>>>>
>>> I will double check _seed, all the work here is related to it. If it
>>> is defined as 'unsigned int' and all the operations on it with same
>>> type, all the problems (complains from internal tool) should he gone.
>>>> You didn't mention the unrelated change in
>>>> src/share/vm/oops/instanceKlass.hpp
>>> This is for the bug 8030129 which will not fix, but add an assert
>>> here to catch problem in debug binary. Sorry forget to mention this.
>>>
>>> Thanks
>>> Yumin
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks
>>>>> Yumin
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20140210/c1af701c/attachment.html
More information about the hotspot-runtime-dev
mailing list