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

[David Holmes]

On 16/11/2015 7:38 AM, Dmitry Dmitriev wrote:
> Hi Gerard,
>
> Several comments:
> 1) I beleive that David mean UINT_MAX, i.e. without "x", because it
> always 32-bit and can avoid overflow of resulting value. Thus, I think
> that upper range should be "max_juint".

<sigh> I missed the 'x' on the flag type. When I said UINT_MAX I was 
thinking the flag was a UINT. UINTX_MAX can lead to overflow as you 
pointed out much earlier - sorry.

> 2) Also, in revision 0 you correct using of ErrorLogTimeout to match
> definition by removing "60" from call to the "os::sleep" in
> src/share/vm/runtime/thread.cpp. Did something changed between rev0 and
> rev1?
>
> 3) Also, David suggested to wrap constants in multiplication in
> src/share/vm/runtime/thread.cpp into CONST64 to avoid possible
> overflows, i.e.:
> os::sleep(this, ErrorLogTimeout * CONST64(1000), false);

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.

David
-----

> Thanks,
> Dmitry
>
> On 12.11.2015 21:26, gerard ziemski wrote:
>> hi all,
>>
>> I have updated the fix with rev1, incorporating the feedback from David:
>>
>> - Use trivial range
>>
>>
>> http://cr.openjdk.java.net/~gziemski/8141641_rev1
>>
>>
>> cheers
>>
>> On 11/10/2015 01:34 PM, gerard ziemski wrote:
>>>
>>> hi all,
>>>
>>> Please review this small fix for JEP-245, which implements range for
>>> the last new runtime flag.
>>>
>>>      bug: https://bugs.openjdk.java.net/browse/JDK-8141641
>>>   webrev: http://cr.openjdk.java.net/~gziemski/8141641_rev0
>>>
>>>
>>> Thank you.
>