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

Docs for Duration are lacking #39949

Closed
ghost opened this issue Feb 19, 2017 · 13 comments
Closed

Docs for Duration are lacking #39949

ghost opened this issue Feb 19, 2017 · 13 comments
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ghost
Copy link

ghost commented Feb 19, 2017

The most common use of Duration for me is benchmarking, and it's always a very unpleasant experience. This is what I typically write:

let now = Instant::now();
// Do something...
let elapsed = now.elapsed();
println!("{:?}", elapsed);

Output: Duration { secs: 1, nanos: 59219906 }
Wait, how much time has elapsed? That's terribly unreadable... :(

The docs for Duration are not very helpful either. When you first see it, Duration is pretty confusing, mostly because of the as_secs/subsec_nanos split.

Then I take a look at the docs for Instant, but the examples show how to print the seconds part only.

Anyways, all I want to do is just print how much time has elapsed without squinting to connect secs and nanos together. Finally, I arrive at this horrible solution:

println!("seconds: {:.3}", elapsed.as_secs() as f64 + elapsed.subsec_nanos() as f64 / 1e9);

Three ways of fixing this come to my mind:

  1. Add a function to Duration that converts it to a (lossy) f64 format.
  2. Add examples to the docs for Instant and Duration that demonstrate how to print the duration time in a human readable form (e.g. total seconds with precision to 3 decimals).
  3. Change the Debug formatting for Duration and print it like this: Duration(secs.nanos: 1.059219906) (note the leading zero). Or maybe even Duration { secs: 1, nanos: 059219906 } (again, the leading zero).

What do you think?

@steveklabnik
Copy link
Member

This seems like just as much a question for @rust-lang/libs as it does @rust-lang/docs

@ranma42
Copy link
Contributor

ranma42 commented Feb 19, 2017

See also #38475 (another call for solution 1).

As an alternative to 3, there were some attempts at implementing Display for Duration (see #29146), but they were rejected because is seems hard to have "one true way" to display Duration values across a wide range of values.

@ghost
Copy link
Author

ghost commented Feb 19, 2017

How about this non-lossy function?

fn as_nanos(&self) -> u128 {
    self.secs as u128 * 1_000_000_000 + self.nanos as u128
}

Printing:

println!("{:.3}", duration.as_nanos() as f64 / 1e9);

@ranma42
Copy link
Contributor

ranma42 commented Feb 19, 2017

The to_nanos function was discussed here #35868.

It looks like several Duration-related items were blocked because of the lack of i128 (see also #32515). Maybe this is a good time to discuss them again? Some experience has probably been gathered through the https://github.com/sfackler/rust-time2 crate.

@sfackler
Copy link
Member

They were not blocked on i128, they're blocked on someone writing up a proposal for what specifically to do. i128 returns are on option.

@steveklabnik steveklabnik added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools P-medium Medium priority and removed A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Mar 10, 2017
@steveklabnik
Copy link
Member

Docs team triage: adding p-medium for option 2.

@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-docs labels Mar 24, 2017
@dhardy
Copy link
Contributor

dhardy commented Mar 29, 2017

What's wrong with the first proposal anyway? Why not just use an f64?

fn as_secs_f64(&self) -> f64 {
    self.as_secs() as f64 + self.subsec_nanos() as f64 * 1e-9
}

(f64 may be "lossy" but it's often still far more precise than required, and this isn't the only way of getting data out of Duration anyway.)

Alternatively there could be something like

fn display(&self, decimal_places: u32) -> DisplayDuration { ... }

println!("Duration: {}", dur.display(3));

@dhardy
Copy link
Contributor

dhardy commented Mar 30, 2017

@stjepang mentioned that f64 is lossy. Looks like he deleted that comment. But it made me question how much precision f64 has: 53 bits.

There are just under 32*10^6 seconds in a year. That's less than 2^25.
Duration has a precision of nanoseconds (although it's not clear how much of this precision is actually used). 1 ns = 10^9 s < 2^30 s. So nanosecond accuracy over a year would require up to 55 bits.

Put another way, an f64 can represent nanosecond accuracy for 104 days, microsecond accuracy for 285 years, and millisecond accuracy for 285,000 years.

Is it possible that some application will require more accuracy than that? Perhaps, but it's going to be some pretty special application and I'd be surprised if Duration would get used for that in the first case (besides, we're not stopping anyone from using higher-precision output from Duration). If you require nanosecond accuracy over a year, your average CPU timer isn't accurate enough anyway.

@ghost
Copy link
Author

ghost commented Mar 30, 2017

@dhardy You raise a fair point about accuracy requirements in practice. Could you elaborate why you dislike as_nanos returning u128?

Also, what do you think about this API?
Converted to Rust it may look something like:

fn secs(&self) -> u64;  // maybe secs_part, as in "the seconds part of the duration"
fn nanos(&self) -> u32; // maybe nanos_part, as in "the nanos part of the duration"
fn total_secs(&self) -> f64;
fn total_nanos(&self) -> f64;

@dhardy
Copy link
Contributor

dhardy commented Mar 30, 2017

@stjepang I have nothing against the existence of fn as_nanos(&self) -> u128, but it's not a satisfactory solution to easy printing of a duration:

println!("Time: {:.2}s", dur.as_secs_f64());
println!("Time: {:.2}s", dur.as_nanos() as f64 * 1e-9);
println!("Time: {}s", dur.display_prec(2));

The latter is something I implemented using a trait as a local solution, but could also be a good solution here.

@frewsxcv
Copy link
Member

frewsxcv commented May 3, 2017

Opened a PR that adds a doc example alongside Duration::as_secs that demonstrates calculating total number of seconds: #41720

frewsxcv added a commit to frewsxcv/rust that referenced this issue May 3, 2017
…teveklabnik

Improvements to `std::time::Duration` doc examples.

Opened primarily for the last commit, in response to comments in rust-lang#39949.
frewsxcv added a commit to frewsxcv/rust that referenced this issue May 3, 2017
…teveklabnik

Improvements to `std::time::Duration` doc examples.

Opened primarily for the last commit, in response to comments in rust-lang#39949.
@frewsxcv
Copy link
Member

frewsxcv commented May 4, 2017

Is there anything actionable left to do here from a docs standpoint? Seems like the remaining discussion in the GitHub Issue is related to new functionality on Duration, which is more a libs thing, less of a docs thing

@steveklabnik
Copy link
Member

From #39949 (comment), looks like there's other issues for the other libs parts of this. Let's close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants