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

[Kim Barrett]

> 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.

(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.

>> 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.

(2) There is no documented monotonicity guarantee for System.nanoTime,
nor any hint of a possible issue there, which surprised me.