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 for Duration::span #27799

Closed
alexcrichton opened this issue Aug 13, 2015 · 9 comments · Fixed by #30187
Closed

Tracking issue for Duration::span #27799

alexcrichton opened this issue Aug 13, 2015 · 9 comments · Fixed by #30187
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

This is a tracking issue for the unstable duration_span feature of the standard library. Known open questions are:

  • Is this the right place for this API?
  • Is this the right name?
  • Should the closure be able to return a value?
  • Should various other system clocks be exposed?
@alexcrichton alexcrichton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Aug 13, 2015
@SimonSapin
Copy link
Contributor

Some previous discussion in rust-lang/rfcs#1243:

  • It’s probably not the right place, just like std::usize is not the right place for Iterator::count even though it returns a usize.
  • I don’t have a strong opinion on the name. timeit was suggested. Maybe time_it is closer to usual naming conventions?
  • Allowing the closure to return a value makes the API awkward if the duration is also a return value. The closure can assign to a close-over variable if needed.
  • I don’t know about differences between clocks. I kinda like not having a to make an uninformed choice, so at least there should be a clear default. (Probably one that is most appropriate for benchmarking.)

One thing is that the reason field:

    #[unstable(feature = "duration_span",
               reason = "unsure if this is the right API or whether it should \
                         wait for a more general \"moment in time\" \
                         abstraction")]

seems to suggest that Duration::span could be removed entirely. IMO even if a "moment in time" abstraction is added, typing let duration = { let start = now(); f(); now() - start }; every time gets tedious very quickly, and a closure-based API should still be provided.

@SimonSapin
Copy link
Contributor

I suggest either:

  1. Moving Duration::span to a free function std::time::span and stabilize in 1.5;

  2. Or moving it to an associated function of a new std::time::Timer type like this:

    #[derive(Copy, Clone)]
    pub struct Timer {
        start_t: SteadyTime,
    }
    
    impl Timer {
        pub fn start() -> Timer {
            Timer {
                start_t: SteadyTime::now(),
            }
        }
    
        pub fn stop(&self) -> Duration {
            &SteadyTime::now() - &self.start_t
        }
    
        pub fn span<F>(f: F) -> Duration where F: FnOnce() {
            let timer = Timer::start();
            f();
            timer.stop()
        }
    }

    (Stability markers omitted for brevity.)

    The idea is to generalize span to cases where the start and end point are not necessarily in the same stack frame, so a closure doesn’t work. Admittedly, this is theoretical. (I haven’t actually needed this myself so far.)

@rustonaut
Copy link

I don't think span is the right name it sounds like it would span a new thread to run and (time) measure the closure.

It is documented as Runs a closure, returning the duration.. so maybe run_and_measure or just measure would be better.

I also agree that it is kind of misplaced. The options provided by @SimonSapin do look good, through maybe a full Timer class is more suited for a small Library (witch also might have some more complex Timers, like hierarchical ones).

@SimonSapin
Copy link
Contributor

Agreed on changing the span name to something else. Python has a timeit module and function, but measure is ok too.

This "full class" is really just an opaque data type to encapsulate a start timestamp. Maybe we can just expose SteadyTime instead (adding Clone and maybe Copy to the existing now() -> SteadyTime method and Sub impl as its only API).

@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 30, 2015

Allowing the closure to return a value makes the API awkward if the duration is also a return value. The closure can assign to a close-over variable if needed.

Assigning to a closed-over variable is pretty ugly too, unfortunately.

@softprops
Copy link

If stablized, it may be useful to return a tuple of both the result of the closure run and the duration.

let (value, duration) = Duration::span(thunk) 

@softprops
Copy link

Alt name suggestion Duration::elapsed

@alexcrichton
Copy link
Member Author

Nominating for 1.6 discussion

@alexcrichton
Copy link
Member Author

🔔 This issue is now entering its cycle-long final comment period for deprecation 🔔

It's likely that rust-lang/rfcs#1288 will land during this cycle in which case using Instant to measure time will be the "more correct" way to do so. This function will not be deprecated, however, until Instant has landed.

@alexcrichton alexcrichton added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed I-nominated labels Nov 5, 2015
alexcrichton added a commit to alexcrichton/rust that referenced this issue Dec 5, 2015
This commit is the standard API stabilization commit for the 1.6 release cycle.
The list of issues and APIs below have all been through their cycle-long FCP and
the libs team decisions are listed below

Stabilized APIs

* `Read::read_exact`
* `ErrorKind::UnexpectedEof` (renamed from `UnexpectedEOF`)
* libcore -- this was a bit of a nuanced stabilization, the crate itself is now
  marked as `#[stable]` and the methods appearing via traits for primitives like
  `char` and `str` are now also marked as stable. Note that the extension traits
  themeselves are marked as unstable as they're imported via the prelude. The
  `try!` macro was also moved from the standard library into libcore to have the
  same interface. Otherwise the functions all have copied stability from the
  standard library now.
* The `#![no_std]` attribute
* `fs::DirBuilder`
* `fs::DirBuilder::new`
* `fs::DirBuilder::recursive`
* `fs::DirBuilder::create`
* `os::unix::fs::DirBuilderExt`
* `os::unix::fs::DirBuilderExt::mode`
* `vec::Drain`
* `vec::Vec::drain`
* `string::Drain`
* `string::String::drain`
* `vec_deque::Drain`
* `vec_deque::VecDeque::drain`
* `collections::hash_map::Drain`
* `collections::hash_map::HashMap::drain`
* `collections::hash_set::Drain`
* `collections::hash_set::HashSet::drain`
* `collections::binary_heap::Drain`
* `collections::binary_heap::BinaryHeap::drain`
* `Vec::extend_from_slice` (renamed from `push_all`)
* `Mutex::get_mut`
* `Mutex::into_inner`
* `RwLock::get_mut`
* `RwLock::into_inner`
* `Iterator::min_by_key` (renamed from `min_by`)
* `Iterator::max_by_key` (renamed from `max_by`)

Deprecated APIs

* `ErrorKind::UnexpectedEOF` (renamed to `UnexpectedEof`)
* `OsString::from_bytes`
* `OsStr::to_cstring`
* `OsStr::to_bytes`
* `fs::walk_dir` and `fs::WalkDir`
* `path::Components::peek`
* `slice::bytes::MutableByteVector`
* `slice::bytes::copy_memory`
* `Vec::push_all` (renamed to `extend_from_slice`)
* `Duration::span`
* `IpAddr`
* `SocketAddr::ip`
* `Read::tee`
* `io::Tee`
* `Write::broadcast`
* `io::Broadcast`
* `Iterator::min_by` (renamed to `min_by_key`)
* `Iterator::max_by` (renamed to `max_by_key`)
* `net::lookup_addr`

New APIs (still unstable)

* `<[T]>::sort_by_key` (added to mirror `min_by_key`)

Closes rust-lang#27585
Closes rust-lang#27704
Closes rust-lang#27707
Closes rust-lang#27710
Closes rust-lang#27711
Closes rust-lang#27727
Closes rust-lang#27740
Closes rust-lang#27744
Closes rust-lang#27799
Closes rust-lang#27801
cc rust-lang#27801 (doesn't close as `Chars` is still unstable)
Closes rust-lang#28968
bors added a commit that referenced this issue Dec 6, 2015
This commit is the standard API stabilization commit for the 1.6 release cycle.
The list of issues and APIs below have all been through their cycle-long FCP and
the libs team decisions are listed below

Stabilized APIs

* `Read::read_exact`
* `ErrorKind::UnexpectedEof` (renamed from `UnexpectedEOF`)
* libcore -- this was a bit of a nuanced stabilization, the crate itself is now
  marked as `#[stable]` and the methods appearing via traits for primitives like
  `char` and `str` are now also marked as stable. Note that the extension traits
  themeselves are marked as unstable as they're imported via the prelude. The
  `try!` macro was also moved from the standard library into libcore to have the
  same interface. Otherwise the functions all have copied stability from the
  standard library now.
* `fs::DirBuilder`
* `fs::DirBuilder::new`
* `fs::DirBuilder::recursive`
* `fs::DirBuilder::create`
* `os::unix::fs::DirBuilderExt`
* `os::unix::fs::DirBuilderExt::mode`
* `vec::Drain`
* `vec::Vec::drain`
* `string::Drain`
* `string::String::drain`
* `vec_deque::Drain`
* `vec_deque::VecDeque::drain`
* `collections::hash_map::Drain`
* `collections::hash_map::HashMap::drain`
* `collections::hash_set::Drain`
* `collections::hash_set::HashSet::drain`
* `collections::binary_heap::Drain`
* `collections::binary_heap::BinaryHeap::drain`
* `Vec::extend_from_slice` (renamed from `push_all`)
* `Mutex::get_mut`
* `Mutex::into_inner`
* `RwLock::get_mut`
* `RwLock::into_inner`
* `Iterator::min_by_key` (renamed from `min_by`)
* `Iterator::max_by_key` (renamed from `max_by`)

Deprecated APIs

* `ErrorKind::UnexpectedEOF` (renamed to `UnexpectedEof`)
* `OsString::from_bytes`
* `OsStr::to_cstring`
* `OsStr::to_bytes`
* `fs::walk_dir` and `fs::WalkDir`
* `path::Components::peek`
* `slice::bytes::MutableByteVector`
* `slice::bytes::copy_memory`
* `Vec::push_all` (renamed to `extend_from_slice`)
* `Duration::span`
* `IpAddr`
* `SocketAddr::ip`
* `Read::tee`
* `io::Tee`
* `Write::broadcast`
* `io::Broadcast`
* `Iterator::min_by` (renamed to `min_by_key`)
* `Iterator::max_by` (renamed to `max_by_key`)
* `net::lookup_addr`

New APIs (still unstable)

* `<[T]>::sort_by_key` (added to mirror `min_by_key`)

Closes #27585
Closes #27704
Closes #27707
Closes #27710
Closes #27711
Closes #27727
Closes #27740
Closes #27744
Closes #27799
Closes #27801
cc #27801 (doesn't close as `Chars` is still unstable)
Closes #28968
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. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. 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.

5 participants