RFR 8068730: Increase the precision of the implementation of java.time.Clock.systemUTC()

[David Holmes]

On 12/01/2015 10:00 PM, Daniel Fuchs wrote:
> On 12/01/15 03:20, David Holmes wrote:
>> Hi Daniel,
>>
>> Looking at the hotspot part ...
>>
>> On 10/01/2015 2:56 AM, Daniel Fuchs wrote:
>
> Hi David,
>
> [...]
>
>>> http://cr.openjdk.java.net/~dfuchs/webrev_8068730/webrev.00/
>>>
>>> Some more details on the patch:
>>>
>>> native (hotspot) side:
>>>
>>>   - adds a new method to the os class:
>>>     os::javaTimeSystemUTC(jlong &seconds, jlong &nanos)
>>>     which allows to get the time in the form of a number
>>>     of seconds and number of nanoseconds
>>>     (see os.hpp and various os_<os>.cpp files)
>>
>> Everything seems consistent with javaTimeMillis code. Only nit on
>> Windows is that windows_time_stamp isn't a very descriptive name (no
>> idea what it returns) - perhaps windows_time_as_micros and adjust the
>> calculations accordingly?
>
> I'm not a big fan of the current name either.
> I would gladly rename it. I did think of
> windows_to_java_time_micros, but it actually returns tenth of
> micros - so it would be lying...
> Is there a good name for 'tenth of micros'?

hundreds of nanos? :)

> Otherwise - I assume I could rename it into:
> windows_to_java_time_max_precision
> Would that be a better name?
> Or would windows_time_as_tenth_of_micros be better?

The windows_ticks name seems adequate.

>>>   - adds a JVM_GetNanoTimeAdjustment method, which takes
>>>     an offset (in seconds) as parameter and returns a
>>>     nano time adjustment compared to the offset.
>>>     Calls os::javaTimeSystemUTC to compute the adjustment
>>>     (see jvm.cpp)
>>
>> IIUC JVM_GetNanoTimeAdjustment(jlong offset_secs) returns the difference
>> in nanoseconds between current UTC time (from Epoch) and the UTC time
>> since the Epoch that offset_secs represents - is that right?
>
> Yes. Exactly.
>
>> I don't find the name particularly intuitive
>
> Well - I borrowed the name from the java time API.
> Instant has a factory method that takes a number of seconds
> and a 'nanoAdjustement':
> http://docs.oracle.com/javase/8/docs/api/java/time/Instant.html#ofEpochSecond-long-long-
>
>
>> and I guess you renamed it
>> somewhere along the way as the mapfiles are no longer in alphabetical
>> order (but would be if this was JVM_GetTimeXXXX). So please fix mapfiles.
>
> You have an eagle's eye ;-)
> Yes - in my early prototype it was named JVM_GetTimeStamp - which was
> not a great name.
> Mapfiles fixed. Thanks for spotting that!
>
>>> java (jdk) side:
>>>
>>>   - adds a native sun.misc.VM.getNanoTimeAdjustment method
>>>     (which is bound to JVM_GetNanoTimeAdjustment)
>>>     (see VM.java and VM.c)
>>
>> Just curious: why the indirection through sun.misc.VM?
>
> In my early prototype I didn't want to add new .c files,
> and so I looked for a home for this new method.
> When I asked around whether adding it to sun.misc.VM would
> be appropriate nobody seemed too shocked.
>
> If you think it's better I could make it a private static
> method in java.time.Clock - but then I'd have to change
> the new unit test for that (GetNanoTimeAdjustment.java) to
> use reflection (and would probably also need to add an
> "initialize" native method to java.time.Clock as well).

I was just curious, I'm not aware of any general policy on the core-libs 
side regarding placement of native methods.

> Here is the new webrev with Stephen & your feedback included
> http://cr.openjdk.java.net/~dfuchs/webrev_8068730/webrev.01/
>
> (windows_time_stamp not renamed yet)

I checked the updated webrev.02 and everything looks good on the hotspot 
side.

What are your plans for pushing this?

David
-----

> Thanks a lot David!
>
> -- daniel
>
>>
>> Cheers,
>> David
>>
>>>   - modifies java.time.Clock.SystemClock to call the new
>>>     sun.misc.VM.getNanoTimeAdjustment instead of
>>>     System.currentTimeMillis.
>>>
>>>   - fixes java.util.Formatter - which wasn't expecting
>>>     greater than millisecond precision.
>>>
>>> testing:
>>>
>>>   - A new test is added to test sun.misc.VM.getNanoTimeAdjustment
>>>     In particular it checks the edge cases.
>>>   - New tests are added to TestClock_System.java to check for
>>>     the increased precision.
>>>     There is also a test for the edge cases there.
>>>   - Some java.time tests where tripped by the increased precision,
>>>     and had to be modified to take that into account
>>>
>>> Note: comparing the new os::javaTimeSystemUTC with os::javaTimeMillis
>>>        can help in reviewing the changes.
>>>
>>> best regards,
>>>
>>> -- daniel
>