time: delay the Arc::clone until registering timer

[ADD-SP]

This reworks the #7461 for less diff, also blocks #7467.

Blocked by #7473 (comment),

Background

This improvement was found while working on the delayed cancellation (#7384),
Since I don't like to include a un-relevant change into a big patch, I made it a separate commit

This might be a low-hanging fruit.

Motivation

tokio/tokio/src/time/sleep.rs

Lines 250 to 256 in 0a3fe46

	pub(crate) fn new_timeout(
	deadline: Instant,
	location: Option<&'static Location<'static>>,
	) -> Sleep {
	use crate::runtime::scheduler;
	let handle = scheduler::Handle::current();
	let entry = TimerEntry::new(handle, deadline);

The current implementation always clone the scheduler::Handle for each timer, even this timer is not registered.

There are two usage of this handle for timer:

1. Ensure the time driver is enabled.
   

   tokio/tokio/src/runtime/time/entry.rs

   Lines 480 to 484 in 0a3fe46

   	impl TimerEntry {
   	#[track_caller]
   	pub(crate) fn new(handle: scheduler::Handle, deadline: Instant) -> Self {
   	// Panic if the time driver is not enabled
   	let _ = handle.driver().time();

2. Registering or clear the entry from the global wheel.
   

   tokio/tokio/src/runtime/time/entry.rs

   Lines 590 to 595 in 0a3fe46

   	if reregister {
   	unsafe {
   	self.driver()
   	.reregister(&self.driver.driver().io, tick, inner.into());
   	}
   	}

For (1), A &Handle is enough, no need to make a clone.

For (2), we can delay the .clone() until we are about to register the entry.

Delaying the Arc::clone improves the performance on multi-core machine.

Solution

• Storing the schedule:::Handle in the TimerShared.

Behavior changes

In summary, once the runtime that creates the timer is shutdown,

• For existing impl: the timer will be fired immediately.
• For this PR: the timer will not be fired as usual.

However, if the Runtime ② didn't enable_time, the new impl will panic at the first poll.

Benchmark (AMD64 16 cores)

We need to work out a proper benchmark script for the time subsystem.

[Darksonn]

We will need to consider the changes to behavior. What if creation and first poll happen on two different runtimes? Maybe the change is acceptable, but we should be aware of what we are changing.

[ADD-SP]

We will need to consider the changes to behavior. What if creation and first poll happen on two different runtimes? Maybe the change is acceptable, but we should be aware of what we are changing.

@Darksonn Thanks for you reminder, I forgot to highlight this part, I have updated the PR description to highlight the changed behavior. I may missed other cases, feel free to point it out.

Strictly speaking, this is a breaking change, but I wouldn't expect an reasonable program relies on it. What do you think?

[Darksonn]

I think the most significant change is whether creating a Sleep outside of a runtime panics or not. Right now it panics, but if we delay registration I imagine it would no longer panic.

Going from panic to no panic is an acceptable change IMO, but I think that the reverse would be more concerning because it introduces panics into programs that used to work. This means that the change is acceptable, but difficult to reverse.

[Darksonn]

It looks like there is a test for it:

tokio/tokio/tests/time_sleep.rs

Lines 173 to 181 in 911ab21

	#[test]
	#[should_panic]
	fn creating_sleep_outside_of_context() {
	let now = Instant::now();
	// This creates a delay outside of the context of a mock timer. This tests
	// that it will panic.
	let _fut = time::sleep_until(now + ms(500));
	}

If that still passes with your change, I guess it already panics when created outside of a runtime?

[ADD-SP]

It looks like there is a test for it:

tokio/tokio/tests/time_sleep.rs

Lines 173 to 181 in 911ab21

	#[test]
	#[should_panic]
	fn creating_sleep_outside_of_context() {
	let now = Instant::now();
	// This creates a delay outside of the context of a mock timer. This tests
	// that it will panic.
	let _fut = time::sleep_until(now + ms(500));
	}

If that still passes with your change, I guess it already panics when created outside of a runtime?

Ah, yes, this PR also panic, my mind is a little messy.

tokio/tokio/src/time/sleep.rs

Lines 256 to 259 in 5de7139

	// ensure both scheduler handle and time driver are available,
	// otherwise panic
	let is_time_enabled = scheduler::Handle::with_current(|hdl| hdl.driver().time.is_some());
	assert!(is_time_enabled, "{TIME_DISABLED_ERROR}");

[ADD-SP]

Hi @Darksonn , do you have other concerns about the behavior changes introduced by this PR? I'm not rushing anything, behavior changes are always tricky for projects like tokio, it is valuable to do more discussions.

Since I don't want to block #7467 for a long time, if you would like to do more discussion about the behavior changes, I can rollback #7467 to the existing behavior to make it ready for review.

[Darksonn]

The behavior change is probably okay, but I wonder whether this is really worth it. Yes, the percentage changes are impressive, but in reality they correspond to a few nanoseconds for an atomic increment/decrement.

[ADD-SP]

I wonder whether this is really worth it.

@Darksonn This is a reasonable point, the figures in real world should not look like that, let's hold on the merge button for now.

I think our concern points out that we currently don't have a proper benchmark script for the time subsystem,
I first realized this when I asked (in Discord) an reproducible example of the regression introduced by 1914e1e.

It is the time for me to workout a proper benchmark script, I think it would also be useful for #7467.

I will rollback #7467 to the existing behavior and make it ready for review.

[ADD-SP]

Added the S-blocked label, I need to work out a proper benchmark script for the time subsystem to see if there is a significant improvement in a scenario that more closer to the real world.

[ADD-SP]

Converted to draft, waiting benchmark.