Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added clock_gettime, clock_getres, clock_settime, clock_getcpuclockid #1281

Merged
merged 1 commit into from
Oct 3, 2020

Conversation

xonatius
Copy link
Contributor

@xonatius xonatius commented Aug 1, 2020

Picked up #1100 and added clock_getcpuclockid call as well. Credits to @kevinwern for the initial version.

https://www.man7.org/linux/man-pages/man2/clock_gettime.2.html
https://www.man7.org/linux/man-pages/man3/clock_getcpuclockid.3.html

Closes #1275

@xonatius
Copy link
Contributor Author

@asomers @kamalmarhubi @posborne

Friendly ping for code review, thanks!

src/time.rs Show resolved Hide resolved
src/time.rs Outdated
}

/// Returns time on the clock id
pub fn time(self) -> Result<TimeSpec> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this method is basically the same as std::time::Instant::now, it would be good if the API could be similar. Do you agree? The time method could be renamed now, and we could add convertability between TimeSpec and Instant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Wasn't sure if now() makes sense for pid-based clocks, but I guess it sort of does as we get the time on the pid-based clock at the current moment. I'll update the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking more, it seems like std::time::Instant is pretty limited in its usage and seems to be designed to just get the current monotonically increasing time, rather than all purpose type to represent an arbitrary point in time (i.e. the only way to construct std::time::Instant is to call Instant::now()).

The other problem is that I don't think it makes sense to convert the time process has been running on CPU to a particular point in time since unix epoch. Looking on C timespec documentation I think TimeSpec is closer to std::time::Duration rather than std::time::Instant (or even std::time::SystemTime), as it represents the interval rather than particular point in time.

If that makes sense, I can add convertability between TimeSpec and Duration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, good point. I agree. Duration is more appropriate.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please fix the test failures?

@xonatius
Copy link
Contributor Author

Looks like with a recent change to libc to change some functions from unsafe to safe on linux platform, the usage of those function is now broken across platforms. I'll look into fixing those, probably the best way is to make functions safe on other platforms as well...

@asomers
Copy link
Member

asomers commented Sep 13, 2020

PR #1294 will fix the unused unsafe warnings. However, I like your change better, because it's more finely tuned. Would you consider opening a separate PR just for those?

@xonatius
Copy link
Contributor Author

Looks like that PR is updated to be more finely tuned, so I guess a separate PR is no longer needed. I'll merge and update my changes once that PR is merged.

On a side note, once rust-lang/libc#1891 gets into new release the unsafe blocks and allow(unused_unsafe) won't be needed anymore.

@xonatius xonatius closed this Sep 14, 2020
@xonatius
Copy link
Contributor Author

Clicked the wrong button :/

@xonatius xonatius reopened this Sep 14, 2020
@vi
Copy link
Contributor

vi commented Sep 15, 2020

On a side note, once rust-lang/libc#1891 gets into new release the unsafe blocks and allow(unused_unsafe) won't be needed anymore.

allow(unused_unsafe) with unsafe blocks can still be beneficial to extend the range of supported libc versions.

@xonatius
Copy link
Contributor Author

allow(unused_unsafe) with unsafe blocks can still be beneficial to extend the range of supported libc versions.

Sure, but that probably extends only by 1 version, as this change already bumps up the libc version to the latest one and can't be built without it.

@asomers
Copy link
Member

asomers commented Sep 20, 2020

You'll have to rebase; don't merge master into your branch.

@xonatius
Copy link
Contributor Author

Done, let me know if you prefer me to squash changes into a single commit.

/// Newtype pattern around `clockid_t` (which is just alias). It pervents bugs caused by
/// accidentally passing wrong value.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
pub struct ClockId(clockid_t);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use libc_enum!?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enum won't work for clockid_t returned by clock_getcpuclockid, that's why I had to go with struct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're worried that the OS may add a new clockid that Nix doesn't know about? Ok, that's a fair reason.

src/time.rs Outdated Show resolved Hide resolved
src/time.rs Outdated Show resolved Hide resolved
src/time.rs Outdated Show resolved Hide resolved
src/time.rs Outdated Show resolved Hide resolved
@asomers
Copy link
Member

asomers commented Sep 27, 2020

LGTM! Just squash the commits now.

@xonatius
Copy link
Contributor Author

Done!

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 3, 2020

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for clock_gettime/clock_getcpuclockid
3 participants