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

Tue Apr 15 11:57:56 UTC 2014

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.

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

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
>