RFR (XS) : 8141641: Runtime: implement range for ErrorLogTimeout

[gerard ziemski]

On 11/17/2015 06:10 AM, Dmitry Dmitriev wrote:
> Hi Gerard,
>
> On 17.11.2015 1:34, gerard ziemski wrote:
>>
>>
>> On 11/16/2015 03:45 PM, David Holmes wrote:
>>>>> I'm making a bit of a hash of this review - sorry.
>>>>>
>>>>> I agree with Dmitry that the os::sleep call should be changed as above:
>>>>>
>>>>> os::sleep(this, ErrorLogTimeout * CONST64(1000), false);
>>>>>
>>>>> but as the sleep arg is a jlong we need to limit ErrorLogTimeout to
>>>>> (MAX_JLONG/1000) _but_ that's too big for uintx on
>>>>> 32-bit. Which means the true range is different on 32-bit versus
>>>>> 64-bit. So if I finally get this right we should have
>>>>> the range as:
>>>>>
>>>>> range(0, LP64_ONLY(MAX_JLONG/1000) NOT_LP64(UINTX_MAX))
>>>>>
>>>>> Not sure we have MAX_JLONG defined as a constant. I don't agree with
>>>>> just selecting the 32-bit range as that again
>>>>> becomes an arbitrary constraint on 64-bit.
>>>>
>>>> Since os:sleep() takes jlong (8 bytes long on both 32 and 64 bit), them
>>>> it would appear that the most straightforward solution is to change
>>>> ErrorLogTimeout type from "uintx" to "uint64_t" (also 8 bytes on both 32
>>>> and 64 bit) and then we can have the range simply defined as:
>>>>
>>>> range(0, max_jlong/1000)
>>>>
>>>> And then we can have:
>>>>
>>>> os::sleep(this, ErrorLogTimeout * CONST64(1000), false); // in seconds
>>>>
>>>> Do we agree?
>>>
>>> Not quite. First if ErrorLogTimeout is 64-bit then we don't need to apply CONST64 to 1000 as integer promotion will take
>>> care of it. But we then pass an unsigned 64-bit value where a signed 64-bit value is expected and that will need a cast
>>> to avoid a conversion warning.
>>>
>>> So perhaps ErrorLogTimeout should be int64_t instead - even though only positive values are valid?
>>
>> There is no int64_t allowed in the globals's macro definition - uint64_t seems like the best type we have available
>> here for jlong.
>>
>> So perhaps:
>>
>> os::sleep(this, (jlong)ErrorLogTimeout * 1000, false); // in seconds
>>
>> is the closest/best we can do?
> Yes, looks good to me.
> Should we convert upper range to uint64_t? I.e.: range(0, (uint64_t) max_jlong/1000)

Yes, I think we should in case compiler complains.

Thank you for the review.


cheers