RFR (XS) : 8141641: Runtime: implement range for ErrorLogTimeout
david.holmes at oracle.com
Mon Nov 16 03:29:25 UTC 2015
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
> 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.
> 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
>> 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.
More information about the hotspot-dev