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

timer: introduce delay function shortcut #1440

Merged
merged 1 commit into from
Aug 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions tokio-test/src/clock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,13 @@
//!
//! use tokio::clock;
//! use tokio_test::{assert_ready, assert_pending, task};
//! use tokio_timer::Delay;
//! use tokio_timer::delay;
//!
//! use std::time::Duration;
//!
//! tokio_test::clock::mock(|handle| {
//! let mut task = task::spawn(async {
//! let delay = Delay::new(clock::now() + Duration::from_secs(1));
//! delay.await
//! delay(clock::now() + Duration::from_secs(1)).await
//! });
//!
//! assert_pending!(task.poll());
Expand Down
8 changes: 3 additions & 5 deletions tokio-test/tests/block_on.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

use std::time::{Duration, Instant};
use tokio_test::block_on;
use tokio_timer::Delay;
use tokio_timer::delay;

#[test]
fn async_block() {
Expand All @@ -20,9 +20,7 @@ fn async_fn() {
}

#[test]
fn delay() {
fn test_delay() {
let deadline = Instant::now() + Duration::from_millis(100);
let delay = Delay::new(deadline);

assert_eq!((), block_on(delay));
assert_eq!((), block_on(delay(deadline)));
}
6 changes: 3 additions & 3 deletions tokio-test/tests/clock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@ use std::time::{Duration, Instant};
use tokio_test::clock::MockClock;
use tokio_test::task::MockTask;
use tokio_test::{assert_not_ready, assert_ready};
use tokio_timer::Delay;
use tokio_timer::delay;

#[test]
fn clock() {
let mut mock = MockClock::new();

mock.enter(|handle| {
let deadline = Instant::now() + Duration::from_secs(1);
let mut delay = Delay::new(deadline);
let mut delay = delay(deadline);

assert_not_ready!(delay.poll());

Expand All @@ -31,7 +31,7 @@ fn notify() {
let mut task = MockTask::new();

mock.enter(|handle| {
let mut delay = Delay::new(deadline);
let mut delay = delay(deadline);

task.enter(|| assert_not_ready!(delay.poll()));

Expand Down
2 changes: 1 addition & 1 deletion tokio-timer/src/delay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ impl Delay {
/// Only millisecond level resolution is guaranteed. There is no guarantee
/// as to how the sub-millisecond portion of `deadline` will be handled.
/// `Delay` should not be used for high-resolution timer use cases.
pub fn new(deadline: Instant) -> Delay {
pub(crate) fn new(deadline: Instant) -> Delay {
let registration = Registration::new(deadline, Duration::from_millis(0));

Delay { registration }
Expand Down
7 changes: 6 additions & 1 deletion tokio-timer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,14 @@ pub use timer::{set_default, Timer};

use std::time::{Duration, Instant};

/// Create a Future that completes at `deadline`.
pub fn delay(deadline: Instant) -> Delay {

Choose a reason for hiding this comment

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

Should this be tagged with #[inline]?

Copy link
Member

Choose a reason for hiding this comment

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

The compiler should be smart enough about that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does rust do any cross-crate inlining of monomorphic functions that aren't explicitly marked whatsoever right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to rust-random/rand#472 (comment), it only inlines monomorphic functions only when LTO is used or it's marked explicitly. However, Delay::new wasn't marked inline before, so nothing has really changed, the compiler can inline Delay::new into delay inside the crate and to the outside the situation will stay the same.

I created a binary crate outside of tokio, included tokio-timer, called delay, compiled in release mode and objdump showed that the call to delay is not inlined, but Delay::new was inlined into delay. So the situation should stay as it was before with this PR.

Copy link
Member

Choose a reason for hiding this comment

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

We can change this in a follow up PR

Delay::new(deadline)
}

/// Create a Future that completes in `duration` from now.
pub fn sleep(duration: Duration) -> Delay {
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but it looks like sleep and delay do the same thing now... I added a note to #1261 to figure this out. My inclination is to remove sleep in favor of delay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not exactly the same, delay will wake at a specified time, sleep will wake at now + specified duration. In my experience sleep is actually much more useful, since usually I want to asynchronously "sleep" for some time from now, rather than asynchronously wake up at some specific moment in the future. Maybe the names of the functions may be changed, but sleep sounds reasonable to me, as it is an async counterpart to an actual thread::sleep.

Copy link
Member

Choose a reason for hiding this comment

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

ok, we can keep both for now

Delay::new(Instant::now() + duration)
delay(Instant::now() + duration)
}

// ===== Internal utils =====
Expand Down
Loading