time: use clock_gettime on macos #116238

tamird · 2023-09-28T17:08:57Z

Replace gettimeofday with clock_gettime(CLOCK_REALTIME) on:

all(target_os = "macos", not(target_arch = "aarch64")),
    target_os = "ios",
    target_os = "watchos",
    target_os = "tvos"
))]

gettimeofday was first used in
time-rs/time@cc367ed
which predated the introduction of clock_gettime support in macOS
10.12 Sierra which became the minimum supported version in
58bbca9.

Replace mach_{absolute_time,timebase_info} with
clock_gettime(CLOCK_REALTIME) on:

all(target_os = "macos", not(target_arch = "aarch64")),
    target_os = "ios",
    target_os = "watchos",
    target_os = "tvos"
))]

mach_{absolute_time,timebase_info} were first used in
time-rs/time@cc367ed
which predated the introduction of clock_gettime support in macOS
10.12 Sierra which became the minimum supported version in
58bbca9.

Note that this change was made for aarch64 in
5008a31 which predated 10.12 becoming
the minimum supported version. The discussion took place in
#91417 and in particular
#91417 (comment)
and
#91417 (comment)
are relevant.

This was left over in c043a0e.

rustbot · 2023-09-28T17:09:04Z

Failed to set assignee to alexcrichton: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

tamird · 2023-09-28T19:36:21Z

r? @joshtriplett

Replace `gettimeofday` with `clock_gettime(CLOCK_REALTIME)` on: ``` all(target_os = "macos", not(target_arch = "aarch64")), target_os = "ios", target_os = "watchos", target_os = "tvos" ))] ``` `gettimeofday` was first used in time-rs/time@cc367ed which predated the introduction of `clock_gettime` support in macOS 10.12 Sierra which became the minimum supported version in 58bbca9.

tamird · 2023-09-28T21:46:17Z

cc @thomcc and @hkratz who were involved in #91417 and may generally be knowledgeable about this arcana.

thomcc · 2023-09-28T21:55:00Z

library/std/src/sys/unix/time.rs

-        t: Timespec,
+impl Instant {
+    pub fn now() -> Instant {
+        Instant { t: Timespec::now(libc::CLOCK_MONOTONIC) }


This is a semantic change from the previous version (which used mach_absolute_time, which would be equivalent to CLOCK_UPTIME_RAW).

I would prefer we not make such a semantic change without consideration.

Yep, and we can keep the previous behavior. I called this out in the commit message, but I'll say it again here: I could not figure out from #91417 why the move to CLOCK_UPTIME_RAW was made.

EDIT: put another way: why was it important to keep the same values as mach_absolute_time?

It's because it's equivalent to mach_absolute_time.

To be more specific, there's a real way that mach_absolute_time is the true monotonic kernel timestamp. I used to think we should switch away from it, but so much of the kernel's internals (and even public APIs) are based around it that I think we should have a good reason to do so.

Sounds good. I've updated this, have another look.

thomcc · 2023-09-28T21:55:52Z

I probably have more experience here than josh, so I'll take the review unless they object.

r? thomcc

joshtriplett · 2023-09-28T22:12:38Z

@thomcc By all means. I'm familiar with clock_gettime but I'm not a macOS expert at all.

thomcc · 2023-09-28T22:13:28Z

Yeah, I'm very familiar with the details of the macOS implementation of it.

Replace `mach_{absolute_time,timebase_info}` with `clock_gettime(CLOCK_REALTIME)` on: ``` all(target_os = "macos", not(target_arch = "aarch64")), target_os = "ios", target_os = "watchos", target_os = "tvos" ))] ``` `mach_{absolute_time,timebase_info}` were first used in time-rs/time@cc367ed which predated the introduction of `clock_gettime` support in macOS 10.12 Sierra which became the minimum supported version in 58bbca9. Note that this change was made for aarch64 in 5008a31 which predated 10.12 becoming the minimum supported version. The discussion took place in rust-lang#91417 and in particular rust-lang#91417 (comment) and rust-lang#91417 (comment) are relevant.

tamird · 2023-10-02T14:53:51Z

@thomcc any further comments?

thomcc · 2023-10-02T14:56:29Z

Sorry, I have to verify which clock the new apple guidance wants us to use. I have time this weekend to do it (I had hoped to do it this past weekend, but real life got in the way), but perhaps not before then.

If it helps, I didn't forget about this PR.

tamird · 2023-10-02T15:00:04Z

All good. My understanding is that this PR doesn't change the clock relative to the status quo, though.

thomcc · 2023-10-02T18:15:37Z

Indeed, it does not, however depending on the approach we need to take it might be premature to remove that code. I want to do some research to get a feeling for that, but it should only take a week or so for me to find the time for that. Sorry for the delays.

tamird · 2023-10-09T14:39:49Z

Friendly ping @thomcc - have you had a chance to think about this?

tamird · 2023-10-16T15:31:21Z

Hi @thomcc, are you still interested in reviewing this PR?

thomcc · 2023-10-23T23:45:56Z

Yeah this is fine for now.

@bors r+

bors · 2023-10-23T23:45:58Z

📌 Commit bc30010 has been approved by thomcc

It is now in the queue for this repository.

bors · 2023-10-24T04:15:42Z

⌛ Testing commit bc30010 with merge e918db8...

bors · 2023-10-24T06:10:44Z

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing e918db8 to master...

rust-timer · 2023-10-24T07:54:50Z

Finished benchmarking commit (e918db8): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.6%	[-3.6%, -3.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.6%	[-3.6%, -3.6%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	8.2%	[8.2%, 8.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	6.3%	[1.5%, 11.6%]	6
Improvements ✅ (primary)	-3.4%	[-3.4%, -3.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.4%	[-3.4%, -3.4%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: missing data
Artifact size: 304.36 MiB -> 304.36 MiB (-0.00%)

time: cfg(any(x)) is the same as cfg(x)

f264d28

This was left over in c043a0e.

rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 28, 2023

This comment has been minimized.

Sign in to view

tamird force-pushed the gettimeofday branch from 9033c83 to 41be7bc Compare September 28, 2023 18:56

rustbot assigned joshtriplett Sep 28, 2023

tamird force-pushed the gettimeofday branch from 41be7bc to aafcac8 Compare September 28, 2023 19:38

tamird force-pushed the gettimeofday branch from aafcac8 to b21eb4f Compare September 28, 2023 21:22

tamird changed the title ~~time: use clock_gettime on all unixen~~ time: use clock_gettime on macos Sep 28, 2023

tamird force-pushed the gettimeofday branch from 1ed0fef to 1945d23 Compare September 28, 2023 21:47

This comment has been minimized.

Sign in to view

tamird force-pushed the gettimeofday branch from 1945d23 to 425327a Compare September 28, 2023 21:52

thomcc reviewed Sep 28, 2023

View reviewed changes

rustbot assigned thomcc and unassigned joshtriplett Sep 28, 2023

tamird force-pushed the gettimeofday branch from 425327a to bc30010 Compare September 28, 2023 22:20

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 24, 2023

bors merged commit e918db8 into rust-lang:master Oct 24, 2023
12 checks passed

rustbot added this to the 1.75.0 milestone Oct 24, 2023

bors mentioned this pull request Oct 24, 2023

unix time module now return result #114038

Merged

2 tasks

tamird deleted the gettimeofday branch October 24, 2023 08:31

BlackHoleFox mentioned this pull request Oct 25, 2023

Remove Apple RNG fallbacks and simplify implementation #116319

Merged

github-actions bot mentioned this pull request Oct 31, 2023

Automated pull from upstream master ferrocene/ferrocene#74

Merged

wangrunji0408 mentioned this pull request Jan 5, 2024

fix: fix time on x86_64 macos madsim-rs/madsim#189

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

time: use clock_gettime on macos #116238

time: use clock_gettime on macos #116238

tamird commented Sep 28, 2023 •

edited

Loading

rustbot commented Sep 28, 2023

This comment has been minimized.

tamird commented Sep 28, 2023

tamird commented Sep 28, 2023

This comment has been minimized.

thomcc Sep 28, 2023

tamird Sep 28, 2023 •

edited

Loading

thomcc Sep 28, 2023

thomcc Sep 28, 2023

tamird Sep 28, 2023

thomcc commented Sep 28, 2023

joshtriplett commented Sep 28, 2023

thomcc commented Sep 28, 2023

tamird commented Oct 2, 2023

thomcc commented Oct 2, 2023 •

edited

Loading

tamird commented Oct 2, 2023

thomcc commented Oct 2, 2023 •

edited

Loading

tamird commented Oct 9, 2023

tamird commented Oct 16, 2023

thomcc commented Oct 23, 2023

bors commented Oct 23, 2023

bors commented Oct 24, 2023

bors commented Oct 24, 2023

rust-timer commented Oct 24, 2023

time: use clock_gettime on macos #116238

time: use clock_gettime on macos #116238

Conversation

tamird commented Sep 28, 2023 • edited Loading

rustbot commented Sep 28, 2023

This comment has been minimized.

tamird commented Sep 28, 2023

tamird commented Sep 28, 2023

This comment has been minimized.

Choose a reason for hiding this comment

tamird Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomcc commented Sep 28, 2023

joshtriplett commented Sep 28, 2023

thomcc commented Sep 28, 2023

tamird commented Oct 2, 2023

thomcc commented Oct 2, 2023 • edited Loading

tamird commented Oct 2, 2023

thomcc commented Oct 2, 2023 • edited Loading

tamird commented Oct 9, 2023

tamird commented Oct 16, 2023

thomcc commented Oct 23, 2023

bors commented Oct 23, 2023

bors commented Oct 24, 2023

bors commented Oct 24, 2023

rust-timer commented Oct 24, 2023

Overall result: ✅ improvements - no action needed

tamird commented Sep 28, 2023 •

edited

Loading

tamird Sep 28, 2023 •

edited

Loading

thomcc commented Oct 2, 2023 •

edited

Loading

thomcc commented Oct 2, 2023 •

edited

Loading