RFR (M): 8235741: Inappropriate uses of os::javaTimeMillis()

[David Holmes]

I tried but failed to generate an incremental webrev at:

http://cr.openjdk.java.net/~dholmes/8235741/webrev.v3-incr/

I hadn't qfresh'd and so it contains v2 and v3 changes :(

Full webrev at:

http://cr.openjdk.java.net/~dholmes/8235741/webrev.v3/

Changes:
- conversion macros are now typed functions
- os_posix.cpp had a name clash so fixed that and updated to use the new 
conversion functions

Thanks,
David

On 17/01/2020 12:48 pm, David Holmes wrote:
> Hi Kim,
> 
> On 17/01/2020 11:39 am, Kim Barrett wrote:
>>> On Jan 15, 2020, at 2:12 AM, David Holmes <david.holmes at oracle.com> 
>>> wrote:
>>>> src/hotspot/share/utilities/globalDefinitions.hpp
>>>>   262 // time unit conversion macros
>>>>   263
>>>>   264 #define NANOS_TO_MILLIS(ns) ((ns) / NANOSECS_PER_MILLISEC)
>>>>   265 #define MILLIS_TO_NANOS(ms) ((ms) * NANOSECS_PER_MILLISEC)
>>>> Why are these macros, rather than (template) functions?
>>>
>>> Just because I just wanted a simple textual replacement to make it 
>>> clearer that I'm converting from millis to nanos or vice versa. I 
>>> reach for macros for such simple cases.
>>>
>>>> Also, depending on the type and value of ms, MILLIS_TO_NANOS could
>>>> easily overflow, e.g. if ms type is a 32 bit type with a value of more
>>>> than ~4 seconds.  (I checked the two uses, and they happen to be okay.)
>>>
>>> These are not trying to be mathematically sound. The conversion from 
>>> millis to nanos is used in two cases:
>>>
>>> 1. Converting a current timestamp in ms to ns. Unless the current 
>>> time is set far in the future I don't think we have any issue with 
>>> overflow of  such a value.
>>>
>>> 2. converting an elapsed time in ms to ns. These will be small values 
>>> so no overflow is possible.
>>
>> In this case, "not trying to be mathematically sound" is equivalent to
>> actively but subtly dangerous to use, because of possible overflow
>> leading to either UB or catastrophically wrong results.
>>
>> Consider "int millis = 10 * 1000;", e.g. 10 seconds worth of
>> milliseconds.  I hope you will agree that this falls under your case
>> 2, e.g. that 10 seconds is not an especially big interval.  I hope you
>> will also agree that declaring a variable that holds a millisecond
>> interval as an int is reasonable in some contexts. Applying
>> MILLIS_TO_NANOS with the jint-typed NANOSECS_PER_MILLISEC, and the
>> result about 1.4 seconds worth of nanoseconds, assuming the compiler
>> doesn't do something strange because of the UB on integer overflow.
> 
> So your concern is that someone will use the macros outside of existing 
> usage contexts (where time values always using 64-bit variables) and 
> thus introduce potential overflow bugs. Okay I will use the typed 
> functions.
> 
>> (This is all in addition to the Style Guide's admonishment to use
>> inline functions rather than macros.  Macros should only be used for
>> syntactic extensions.)
> 
> 
>>>> inline int64_t nanos_to_millis(int64_t ns) {
>>>>    return ns / NANOSECS_PER_MILLISECOND;
>>>> }
>>>> inline int64_t millis_to_nanos(int64_t ms) {
>>>>    return ms * NANOSECONDS_PER_MILLISEC;
>>>> }
>>>> Also, the names don't suggest time conversions, but potentially
>>>> arbitrary unit conversions, e.g. between something in NANOUNITS and
>>>> something in MILLIUNITS.
>>>
>>> They don't have to be time conversions - the calculation is unit-less 
>>> in practice. The fact we have NANOSEC_PER_MILLISECOND et al is just 
>>> an artifact of introducing those values for timeout 
>>> calculations/conversions - it could just be NANOS_PER_MILLI etc
>>
>> If they aren't time conversions then I don't think they should be
>> using the time factors; just use NANOUNITS/MILLIUNITS.
> 
> Okay.
> 
> 
>>>> Regarding this from the audit:
>>>> --- begin ---
>>>> ./share/gc/parallel/psParallelCompact.cpp: // os::javaTimeMillis() 
>>>> does not guarantee monotonicity.
>>>> ...
>>>> ./share/gc/shared/referenceProcessor.cpp: // os::javaTimeMillis() 
>>>> does not guarantee monotonicity.
>>>> These are all describing why the subsequent code uses javaTimeNanos 
>>>> not javaTimeMillis.
>>>> --- end ---
>>>> Do we really still support platforms that don't have a monotonic
>>>> clock?  I guess we appear to at least try.  But it's really wrong that
>>>> callers of os::javaTimeNanos should even think they need to cope with
>>>> that function being non-monotonic.
>>>> Hm, I always thought System.nanoTime() was a monotonic clock, but I
>>>> don't see any such guarantee. So I guess Java just doesn't have such a
>>>> thing. Wow!
>>>> So I guess none of this is really relevant to the change at hand 
>>>> after all.
>>>
>>> I think you read the comments the wrong way round. The code uses 
>>> javaTimeNanos not javaTimeMillis because javaTimeMillis is not 
>>> monotonic and the code wants a monotonic clock. These comments were 
>>> mostly inserted when the incorrect use of javaTimeMillis was replaced 
>>> with javaTimeNanos.
>>
>> No, I did not read the comments the wrong way around.
>>
>> I was pointing out that
>>
>> (1) Some uses of os::javaTimeNanos attempt to cope with a possible
>> lack of monotonicity, while some do not and may be assuming or even
>> requiring monotonicity.  I'm wondering whether the coping attempts are
>> dead code, or instead, the latter are bugs.  That is, do we require
>> all platforms to provide a monotonic os::javaTimeNanos or not?
>> Personally, I think in today's world we should make such a requirement.
> 
> I assume you are looking at code where we explicitly ensure that time 
> never jumps backwards? This is historical. The sad fact is that over the 
> past 20+ years we have seen bug after bug after bug in the OS and 
> virtualization layers that caused the system monotonic clock source to 
> actually not behave monotonically. These bugs were prevalent enough in 
> some situations that we had to implement the workarounds in the JVM. 
> These workarounds have had costs and have varied over time. Ensuring 
> that javaTimeNanos() itself is guaranteed monotonic imposes too high a 
> cost - see related discussions in:
> 
> https://bugs.openjdk.java.net/browse/JDK-6864866
> https://bugs.openjdk.java.net/browse/JDK-6784100
> 
> regarding the situation on Solaris. In general we try to trust the 
> system monotonic clock and deal with issues if they arise.
> 
> So in a bugfree world:
> - os::javaTimeNanos is "guaranteed" monotonic (assuming the underlying 
> monotonic clock exists on that platform)
> - os::currentTimeMillis() is not monotonic as it is (required to be) 
> subject to time-of-day adjustments (e.g. ntp)
> 
>> (2) There is no documented monotonicity guarantee for System.nanoTime,
>> nor any hint of a possible issue there, which surprised me.
> 
> Again historical. Back in 2003 when we (JSR166 EG) specified this we 
> also had to consider the implications for Java ME and the constrained 
> environments in which it would execute, as well as the various 
> environments supported by Java SE**. As a practical consideration it was 
> necessary that a valid/conforming implementation of nanoTime() could be 
> currentTimeMillis()*1000000 and without making heroic, and costly, 
> efforts to ensure monotonicity - which was basically left as a quality 
> of implementation issue.
> 
> ** Windows versions without QueryPerformanceCounter; Linux 2.x kernels 
> without CLOCK_MONOTONIC
> 
> Thanks,
> David
> ----
> 
>>