This is an archive of the discontinued LLVM Phabricator instance.

[libc++][Apple] Use CLOCK_MONOTONIC_RAW instead of CLOCK_UPTIME_RAW for steady_clock
ClosedPublic

Authored by ldionne on Feb 10 2020, 9:55 AM.

Details

Summary

In D27429, we switched the Apple implementation of steady_clock::now()
from clock_gettime(CLOCK_MONOTONIC) to clock_gettime(CLOCK_UPTIME_RAW).
The purpose was to get nanosecond precision, and also to improve the
performance of the implementation.

However, it appears that CLOCK_UPTIME_RAW does not satisfy the requirements
of the Standard, since it is not strictly speaking monotonic. Indeed, the
clock does not increment while the system is asleep, which had been
mentioned in D27429 but somehow not addressed.

This patch switches to CLOCK_MONOTONIC_RAW, which is monotonic, increased
during sleep, and also has nanosecond precision.

https://llvm.org/PR44773

Diff Detail

Event Timeline

ldionne created this revision.Feb 10 2020, 9:55 AM

Specifically, I'm looking to understand why Eric's comment at that time (https://reviews.llvm.org/D27429#618296) about using CLOCK_MONOTONIC_RAW wasn't acted upon -- did we simply overlook it or is there a reason why CLOCK_MONOTONIC_RAW is not a suitable implementation?

Specifically, I'm looking to understand why Eric's comment at that time (https://reviews.llvm.org/D27429#618296) about using CLOCK_MONOTONIC_RAW wasn't acted upon -- did we simply overlook it or is there a reason why CLOCK_MONOTONIC_RAW is not a suitable implementation?

Looking at the discussion I think Howard had concerns with CLOCK_MONOTONIC. They probably didn't apply to CLOCK_MONOTONIC_RAW, but we didn't test that.

One benefit of using CLOCK_UPTIME_RAW is that it matches libc++'s historical behaviour on Apple platforms, since it exactly matches mach_absolute_time(). The main purpose of the above commit was to fix a binary compatibility issue caused by moving away from mach_absolute_time() and this was the conservative fix.

Specifically, I'm looking to understand why Eric's comment at that time (https://reviews.llvm.org/D27429#618296) about using CLOCK_MONOTONIC_RAW wasn't acted upon -- did we simply overlook it or is there a reason why CLOCK_MONOTONIC_RAW is not a suitable implementation?

Looking at the discussion I think Howard had concerns with CLOCK_MONOTONIC. They probably didn't apply to CLOCK_MONOTONIC_RAW, but we didn't test that.

One benefit of using CLOCK_UPTIME_RAW is that it matches libc++'s historical behaviour on Apple platforms, since it exactly matches mach_absolute_time(). The main purpose of the above commit was to fix a binary compatibility issue caused by moving away from mach_absolute_time() and this was the conservative fix.

I still do think that CLOCK_MONOTONIC_RAW is the correct behavior w.r.t. the Standard. I'll go ahead and check that it doesn't result in the bincompat issue we were seeing with CLOCK_MONOTONIC.

EricWF accepted this revision.Feb 12 2020, 7:30 AM

LGTM.

I'm not sure why CLOCK_MONOTONIC_RAW wasn't used originally, but that appears to be the correct approach.

This revision is now accepted and ready to land.Feb 12 2020, 7:30 AM

Can we assume that all supported Apple platforms have clock_gettime now? (It was added in 10.12)

If so, can we remove the fallback?

Can we assume that all supported Apple platforms have clock_gettime now? (It was added in 10.12)

If so, can we remove the fallback?

I think it would be fine to remove the fallback, as the only thing that'll do is require macOS >= 10.12 in order to build the libc++ dylib. Since pretty much only vendors build the dylib, I think that's a reasonable thing to do. Will do in a followup patch.

This revision was automatically updated to reflect the committed changes.