RFR: 8033792: AltHashing used jint for imprecise bit shifting
Calvin Cheung
calvin.cheung at oracle.com
Mon Feb 10 11:08:36 PST 2014
Looks good to me.
Calvin
On 2/7/2014 9:08 PM, 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
>>
>
More information about the hotspot-runtime-dev
mailing list