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

[David Holmes]

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

>