RFR: 8040140 System.nanoTime() is slow and non-monotonic on OS X

Tue Apr 15 18:41:39 UTC 2014

It looks good.

Thanks,
Serguei

On 4/15/14 5:21 AM, Staffan Larsen wrote:
> On 15 apr 2014, at 13:57, David Holmes <david.holmes at oracle.com> wrote:
>
>> Hi Staffan,
>>
>> For the solaris changes ...
>>
>> This comment is not accurate from the original code:
>>
>> 1367 // gethrtime can move backwards if read from one cpu and then a different cpu
>> 1368 // getTimeNanos is guaranteed to not move backward on Solaris
>> 1369 // AssumeMonotonicOSTimers can be used to remove this guarantee.
>>
>> can I suggest
>>
>> // gethrtime should be monotonic but has had bugs in the past.
>> // getTimeNanos must be guaranteed not to move backwards, so we add a
>> // check in case gethrtime is buggy.
>> // AssumeMonotonicOSTimers can be set to remove this additional check.
> Fixed.
>
>> With regard to code deletion this seems to be dead code now as well:
>>
>> 350 const int LOCK_BUSY = 1;
>> 351 const int LOCK_FREE = 0;
>> 352 const int LOCK_INVALID = -1;
>>
>> 354 static volatile int max_hrtime_lock = LOCK_FREE;     // Update counter with LSB as lock-in-progress
> Fixed.
>
> New version: http://cr.openjdk.java.net/~sla/8040140/webrev.03/
>
> /Staffan
>
>> Thanks,
>> David
>>
>>
>> On 15/04/2014 9:42 PM, Staffan Larsen wrote:
>>> On 15 apr 2014, at 11:38, David Holmes <david.holmes at oracle.com
>>> <mailto:david.holmes at oracle.com>> wrote:
>>>
>>>> On 15/04/2014 7:20 PM, Staffan Larsen wrote:
>>>>> On 15 apr 2014, at 09:14, David Holmes <david.holmes at oracle.com
>>>>> <mailto:david.holmes at oracle.com>> wrote:
>>>>>
>>>>>> Hi Staffan,
>>>>>>
>>>>>> Generally looks okay.
>>>>>>
>>>>>> os_bsd.cpp still shows the old URL for Dave Dice's article
>>>>> I had forgotten to save the file. :-(
>>>>>
>>>>>> os_solaris.cpp:
>>>>>>
>>>>>> In the Solaris changes there is a lot of old code with inaccurate
>>>>>> comments, but I suppose cleaning that up (oldgetTimeNanos()) is out
>>>>>> of scope. You only added the check for AssumeMonotonicOSTimers in
>>>>>> the supports_cx8 path, but the other path is now dead code.
>>>>> Since supports_cx8() returns true (because SUPPORTS_NATIVE_CX8 is
>>>>> defined) on both solaris sparc and solaris x86, it looks like
>>>>> oldgetTimeNanos() is really dead code. I can remove it as part of
>>>>> this change if that’s ok.
>>>> I'm in favour of removing dead code. :)
>>> Gone!
>>>
>>>>>> globals.hpp
>>>>>>
>>>>>> Do we need to document this only affects OSX and Solaris? (Though
>>>>>> implicitly this acts as-if true on Linux and Windows in the common
>>>>>> case.)
>>>>> I will update the description to:
>>>>>
>>>>>   experimental(bool, AssumeMonotonicOSTimers, false,
>>>>>                        \
>>>>>           "Assume that the OS system timers are monotonic "
>>>>>                 \
>>>>>           "(Solaris and OS X)")
>>>>>                                             \
>>>> Sounds good. Will you close 6864866 as a duplicate of this one?
>>> I will include 6864866 in the hg commit message closing both with the
>>> same fix.
>>>
>>>>>> os.hpp:
>>>>>>
>>>>>> #ifdef TARGET_OS_FAMILY_bsd
>>>>>> # include "jvm_bsd.h"
>>>>>> # include <setjmp.h>
>>>>>> + # include <mach/mach_time.h>
>>>>>> #endif
>>>>>>
>>>>>> I think this include needs to be in a OSX/Apple specific conditional.
>>>>> Changed it to:
>>>>>
>>>>> #ifdef TARGET_OS_FAMILY_bsd
>>>>> # include "jvm_bsd.h"
>>>>> # include <setjmp.h>
>>>>> # ifdef __APPLE__
>>>>> #  include <mach/mach_time.h>
>>>>> # endif
>>>> Ok. I wonder if someone could test this on a non-Apple BSD system? ;-)
>>> Updated review here: http://cr.openjdk.java.net/~sla/8040140/webrev.02/
>>>
>>> Thanks,
>>> /Staffan
>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks,
>>>>> /Staffan
>>>>>
>>>>>> --
>>>>>>
>>>>>> We should really fix the non-monotonic-clock path in the Linux and
>>>>>> Windows implementations too ... but 32-bit is problematic <sigh>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>> On 15/04/2014 4:00 PM, Staffan Larsen wrote:
>>>>>>> Here is an updated webrev with changes to the comments in
>>>>>>> os_bsd.cpp and
>>>>>>> os_solaris.cpp.
>>>>>>> - obs -> obsv
>>>>>>> - fixed URL to blog entry
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~sla/8040140/webrev.01/
>>>>>>>
>>>>>>> /Staffan
>>>>>>>
>>>>>>> On 15 apr 2014, at 07:52, Staffan Larsen <staffan.larsen at oracle.com
>>>>>>> <mailto:staffan.larsen at oracle.com>
>>>>>>> <mailto:staffan.larsen at oracle.com>> wrote:
>>>>>>>
>>>>>>>> On 14 apr 2014, at 21:08, Aleksey Shipilev
>>>>>>>> <aleksey.shipilev at oracle.com <mailto:aleksey.shipilev at oracle.com>
>>>>>>>> <mailto:aleksey.shipilev at oracle.com>> wrote:
>>>>>>>>
>>>>>>>>> On 04/14/2014 06:55 PM, Staffan Larsen wrote:
>>>>>>>>>> mach_absolute_time() is essentially a direct call to RDTSC, but with
>>>>>>>>>> conversion factor to offset for any system sleeps and frequency
>>>>>>>>>> changes. The call returns something that can be converted to
>>>>>>>>>> nanoseconds using information from mach_timebase_info(). Calls to
>>>>>>>>>> mach_absolute_time() do not enter the kernel and are very fast. The
>>>>>>>>>> resulting time has nanosecond precision and as good accuracy as one
>>>>>>>>>> can get.
>>>>>>>>> Some numbers would be good on the public list :) I know the numbers
>>>>>>>>> already, but others on this list don’t.
>>>>>>>> I posted the numbers in the bug, but forgot to say so here...
>>>>>>>>
>>>>>>>>>> Since the value from RDTSC can be subject to drifting between CPUs,
>>>>>>>>>> we implement safeguards for this to make sure we never return a
>>>>>>>>>> lower
>>>>>>>>>> value than the previous values. This adds some overhead to
>>>>>>>>>> nanoTime()
>>>>>>>>>> but guards us against possible bugs in the OS. For users who are
>>>>>>>>>> willing to trust the OS and need the fastest possible calls to
>>>>>>>>>> System.nanoTime(), we add a flag to disable this safeguard:
>>>>>>>>>> -XX:+AssumeMonotonicOSTimers.
>>>>>>>>> I now wonder if this safeguard can produce a stream of exactly
>>>>>>>>> the same
>>>>>>>>> timestamps if local clock is lagging behind. But considering the
>>>>>>>>> alternative of answering the retrograde time, and the observation the
>>>>>>>>> current Mac OS X mach_absolute_time() *appears* monotonic, having
>>>>>>>>> this
>>>>>>>>> safeguard seems OK.
>>>>>>>>>
>>>>>>>>>> webrev: http://cr.openjdk.java.net/~sla/8040140/webrev.00/
>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8040140
>>>>>>>>> This looks good to me.
>>>>>>>> Thanks.
>>>>>>>>
>>>>>>>>> And, since this question will inevitably pop up, do we plan to
>>>>>>>>> bring it
>>>>>>>>> into 8uX? I think many Mac users will be happy about that.
>>>>>>>> I would like to do so, but I would also like to have it sit and bake
>>>>>>>> for a while in 9 before that. I think the 8u20 train has left the
>>>>>>>> station, but perhaps 8u40?
>>>>>>>>
>>>>>>>> /Staffan