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

[Srinivas Ramakrishna]

Hi David --

On Mon, Dec 12, 2011 at 9:28 PM, David Holmes <david.holmes at oracle.com>wrote:

> 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());
>

While the macro reduces the # characters, that was not the intent of my
suggestion...


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

Then, how about os::monotonicTimeMillis() defined as an inline method in
os:: ? One of the reasons for wanting the consolidation
into one place was also so that the commentary would be in one place,
including the part about monotonicity only if and when the
underlying platform guarantees it, which isn't true today on all the
supported platforms.

Anyway, do whatever makes sense wrt proper naming and reducing duplication,
clutter or confusion as the case may be...
thanks.
-- ramki


>
> 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@**oracle.com<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/>
>>>    <http://cr.openjdk.java.net/%**7Ejohnc/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/>
>>>        <http://cr.openjdk.java.net/%**7Ejohnc/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
>>>
>>>
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20111212/7417cab0/attachment.htm>