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

Tracking Issue: Duration::{as_nanos, as_micros, as_millis} #50202

Closed
fintelia opened this issue Apr 24, 2018 · 21 comments · Fixed by #57124
Closed

Tracking Issue: Duration::{as_nanos, as_micros, as_millis} #50202

fintelia opened this issue Apr 24, 2018 · 21 comments · Fixed by #57124
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@fintelia
Copy link
Contributor

Duration has historically lacked a way to get the actual number of nanoseconds it contained as a normal Rust type because u64 was of insufficient range, and f64 of insufficient precision. The u128 type solves both issues, so I propose adding an as_nanos function to expose the capability.

CC: #50167

@pietroalbini pietroalbini added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Apr 26, 2018
@Mark-Simulacrum Mark-Simulacrum changed the title Duration should have an as_nanos function Tracking Issue: Duration::as_nanos May 27, 2018
@sfackler sfackler added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Jun 2, 2018
@sfackler sfackler changed the title Tracking Issue: Duration::as_nanos Tracking Issue: Duration::{as_nanos, as_micros, as_millis} Jun 2, 2018
@gbutler69
Copy link

gbutler69 commented Jul 4, 2018

I still don't understand why Duration is not allowed to be negative. It bugs me that the SystemTime difference returns a Result<Duration,Error> and returns and error if the time from is after the time to. To me, it should just return a negative Duration (Yes, the System Clock can go backwards). I find it odd that Duration does not support a negative amount.

@lnicola
Copy link
Member

lnicola commented Jul 4, 2018

@gbutler69 I think it's about the way you think of Duration. If it's a span of time (like described in the documentation) or an interval, then it must be positive. If it's the distance between two time points, it's negative.

You're probably thinking of a distance instead of an interval. That's not necessarily a bad thing, but it's different from what the type is today, and it will never change due to compatibility concerns.

@gbutler69
Copy link

gbutler69 commented Jul 4, 2018

If it's a span of time (like described in the documentation) or an interval, then it must be positive. If it's the distance between two time points, it's negative.

Here is what I take issue with:

let t1 = std:time:SystemTime.now();
// ....long running computation happening...
// System Clock Time changed back 5 hours (because it was wrong to begin with) outside the program
// ....long running computation completed...
let t2 = std:time:SystemTime.now();

let clock_time_between_events = t2.duration_since( t1 ); // returns an error instead of negative duration, so I must do...
let ( clock_time_between_events, neg_duration ) = match ( clock_time_between_events ) {
    Some(duration) => ( duration, false )
    _ => ( t1.duration_since( t2 ).unwrap(), true )
}

And....DING! DING! DING! ....you know what, now that I've typed out that code, I can't think of any justifiable reason I would actually want that, so, once again, the thoughtfulness of Rust shows itself. You and the designers of Duration were correct. Negative Duration doesn't make sense (even though it feels like it should).

@lnicola
Copy link
Member

lnicola commented Jul 4, 2018

You can use https://doc.rust-lang.org/std/time/struct.Instant.html if you want to measure a duration.

@gbutler69
Copy link

gbutler69 commented Jul 4, 2018

You can use https://doc.rust-lang.org/std/time/struct.Instant.html if you want to measure a duration.

Yes, I'm aware. I guess I think there should be another thing called ClockTimeDifference (that can be neg/pos) and there should be methods on SystemTime that don't return Result<Duration,Error>, but, instead return ClockTimeDifference. It could be as simple as:

struct ClockTimeDifference {
   duration : Duration,
   is_negative : bool
}

@lnicola
Copy link
Member

lnicola commented Jul 4, 2018

It can, but the return type of duration_since will never change, so it's up to you (or a crate) to implement that.

@fintelia
Copy link
Contributor Author

fintelia commented Jul 4, 2018

@gbutler69 @lnicola This is the tracking issue for a feature that adds several specific methods to Duration, not a place to debate general concerns about the Duration type. Your discussion is probably better suited for the internals.rust-lang.org site.

@NatTuck
Copy link

NatTuck commented Jul 6, 2018

Why can't as_millis return an i64?

@sfackler
Copy link
Member

sfackler commented Jul 7, 2018

@NatTuck large durations won't fit into that type.

@rivertam
Copy link
Contributor

rivertam commented Nov 13, 2018

Pardon my ignorance, but why is this unstable? For my usecase, I probably don't need more than a u32, and I really only need millis (though the code I'm porting happens to use microseconds). I'd rather have a u128, but I'm just not sure what to do here for my code which I'm trying to keep as close to fully stable as possible (I want it to be as portable and safe as I can make it).

I know (I think?) I can use d.as_secs() * 1_000_000u64 + d.subsec_micros() as u64, but I'd rather not lose the precision and I think I can always use u128 for all my platforms.

@fintelia
Copy link
Contributor Author

@sfackler is there any reason not to move forward with stabilization on this? @rivertam correctly observes that it has been unstable for quite a while

@sfackler
Copy link
Member

The stability of u128 was the blocker previously, but that's now no longer a problem.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Nov 13, 2018

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Nov 13, 2018
@SimonSapin
Copy link
Contributor

I couldn’t find all the details in this issue, grepping through the code shows that this tracking issue is for:

    pub fn as_millis(&self) -> u128 {}
    pub fn as_micros(&self) -> u128 {}
    pub fn as_nanos(&self) -> u128 {}

@lnicola
Copy link
Member

lnicola commented Nov 14, 2018

This may be a stupid question, but is u128 available on all platforms that Rust supports?

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 14, 2018
@rfcbot
Copy link

rfcbot commented Nov 14, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@sfackler
Copy link
Member

@lnicola Yep, it should be available on all platforms.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 24, 2018
@rfcbot
Copy link

rfcbot commented Nov 24, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@sunjay
Copy link
Member

sunjay commented Dec 24, 2018

Now that final comment period has ended, can these methods be stabilized?

@SimonSapin
Copy link
Contributor

Yes, the next step is a stabilization PR.

@sunjay
Copy link
Member

sunjay commented Dec 25, 2018

Yes, the next step is a stabilization PR.

Awesome! I've made a PR for stabilization here: #57124

Please let me know if it needs any adjustments. I can't wait for this to land! 🎉

bors added a commit that referenced this issue Dec 26, 2018
Stabilize Duration::{as_millis, as_micros, as_nanos}

Fixes #50202. 🎉

This is the stabilization PR for the `duration_as_u128` feature. I have never made one of these before so please let me know if I missed a step. I followed the [guide in the Rust Forge](https://forge.rust-lang.org/stabilization-guide.html) and also found some old stabilization PRs ([1](#57002), [2](#56207)) for similar features to base my work on.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants