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

[Daniel Fuchs]

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

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?

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

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)

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