RFR (S): 7117303: VM uses non-monotonic time source and complains that it is non-monotonic

[David Holmes]

On 13/12/2011 6:06 AM, John Cuthbertson wrote:
> Thanks for looking at the code changes and for the suggestions.
>
> Extending the os API is a good idea. Not sure about the name though -
> how about: javaMonotonicTimeMillis? Also I believe os_solaris.cpp
> already has an internal monotonic version of javaTimeMillis() so there
> are alternatives to using javaTimeNanos() in the implementation.

I'm somewhat opposed to extending the API simply because there are 
already too many time functions. What about a TO_MILLIS macro?

#define TO_MILLIS(nanos) ((nanos)/NANOS_PER_MILLISEC

jlong start = TO_MILLIS(javaTimeNanos());

BTW the "java" in javaTimeMillis and javaTimeNanos identifies these 
methods as being part of the Java API (System.currentTimeMillis and 
System.nanoTime respectively) - so javaMonotonicTimeMillis would not be 
appropriate.

John: you have a recurrent typo in the comments: monton instead of 
monoton (and I mistyped it myself writing that!)

> Thanks for the heads up about the other CR. I'll check.

See 4741166. It wanted to make javaTimeMillis monotonic, which it can 
not be. Pretty much all internal uses of javaTimeMillis (those not part 
of implementing System.currentTimeMillis()) should really be using a 
monotonic time source.

David
-----

> Thanks,
>
> JohnC
>
> On 12/11/11 01:32, Srinivas Ramakrishna wrote:
>> Hi John -- Looks fine. Two minor comments:
>>
>> (1) by defining an os::javaTimeNanosInMillis() you may be able to
>> consolidate the divide by NANOS_IN_MILLISEC to one place instead of it
>> appearing at each use.
>> (2) you might consolidate the commentary about monotonicity into
>> os::javaTimeNanosInMillis(), then at each client simly say "Protect
>> against possible nonmonotonicity"
>>
>> That will reduce the number of repeated lines while still providing
>> all of the commentary in all the relevant places. (By the way, you
>> might want to
>> check if there;s another CR with the same content which might be
>> closed as a dup of this... just a very vague recollection.)
>>
>> reviewed.
>> -- ramki (opendjk: ysr)
>>
>> On Fri, Dec 9, 2011 at 11:30 AM, John Cuthbertson
>> <john.cuthbertson at oracle.com <mailto:john.cuthbertson at oracle.com>> wrote:
>>
>>     Hi Everyone,
>>
>>     I have updated the comments based upon feedback from David Holmes.
>>     A new webrev can be found at:
>>     http://cr.openjdk.java.net/~johnc/7117303/webrev.1/
>>     <http://cr.openjdk.java.net/%7Ejohnc/7117303/webrev.1/>
>>
>>     Thanks,
>>
>>     JohnC
>>
>>
>>     On 12/7/2011 9:59 AM, John Cuthbertson wrote:
>>
>>         Hi Everyone,
>>
>>         Can I have a couple of volunteers review the changes for this
>>         CR? The webrev can be found at:
>>         http://cr.openjdk.java.net/~johnc/7117303/webrev.0/
>>         <http://cr.openjdk.java.net/%7Ejohnc/7117303/webrev.0/>.
>>
>>         Summary:
>>         I replaced the calls to os::javaTimeMillis() in the GC where
>>         we expect monotonicity with calls os::javaTimeNanos(),
>>         converting the result to milliseconds. os::javaTimeNanos(), at
>>         least on some configurations, does guarantee monotonicity and
>>         so is a better alternative. The changes in the os_<*> files
>>         are to make use of the named conversion constants I
>>         added/moved to globalDefinitions.hpp - we seemed to have
>>         multiple names for the same two constants.
>>
>>         Testing: GC test suite on solaris and Linux, NSK tests on
>>         solaris, and jprt.
>>
>>         Thanks,
>>
>>         JohnC
>>
>>
>