-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
time: delay the Arc::clone
of the scheduler::Handle
until registering timer
#7461
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
Conversation
There are two usage of this handle for timer: 1. Ensure the time driver is enabled. 2. Registering or clear the entry from the global wheel. For (1), we just need the `&Handle`, 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. Signed-off-by: ADD-SP <qiqi.zhang@konghq.com>
Signed-off-by: ADD-SP <qiqi.zhang@konghq.com>
Signed-off-by: ADD-SP <qiqi.zhang@konghq.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reviewers
Hiding whitespace reduces diffs.
Signed-off-by: ADD-SP <qiqi.zhang@konghq.com>
Arc::clone
of the scheduler::Handle
until registering timer
Signed-off-by: ADD-SP <qiqi.zhang@konghq.com>
let is_time_enabled = scheduler::Handle::with_current(|hdl| hdl.driver().time.is_some()); | ||
assert!(is_time_enabled, "{TIME_DISABLED_ERROR}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reviewers
Triggering panic inside the closure doesn't report the panic location of the caller, so we need to panic outside the closure.
superseded by #7473 |
blocks #7467
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
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:
tokio/tokio/src/runtime/time/entry.rs
Lines 480 to 484 in 0a3fe46
tokio/tokio/src/runtime/time/entry.rs
Lines 590 to 595 in 0a3fe46
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
&schedule:::Handle
in theTimerShared
.Benchmark (AMD64 16 cores)