time: eliminate timer wheel allocations #6779
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
R-loom-time-driver
tglane
changed the title
tglane
force-pushed
the
6757-timer-wheel-allocations
branch
from
3e12f33 to
083cf36
Compare
tglane
force-pushed
the
6757-timer-wheel-allocations
branch
from
083cf36 to
302a757
Compare
Darksonn
reviewed
Aug 16, 2024
hawkw
reviewed
Aug 16, 2024
tokio/src/runtime/time/mod.rs
Outdated
| self.wheels.len() as u32 | ||
| self.wheels | ||
| .read() | ||
| .expect("Timer wheel shards poisened") |
There was a problem hiding this comment.
It's a bit unfortunate that this can panic. Not because the panic itself is bad, if the lock is poisoned, we should panic, but because panicking code will make this otherwise small and trivially inlined function generate way more code. Perhaps we should move the number of shards outside of the lock, instead? Since it will never change, it should be fine to do that...
There was a problem hiding this comment.
also, a typo:
Suggested change
| .expect("Timer wheel shards poisened") | |
| .expect("Timer wheel shards poisoned") |
There was a problem hiding this comment.
Seems reasonable to put it as a member variable into the Inner struct to avoid the lock here. The increase in size of the struct would probably be ok since it's "only" a u32, so I would agree to do it like that if nobody objects.
There was a problem hiding this comment.
I included the changes in e9c4351.
I also removed the method to get the shard size from the struct ShardedWheel to prevent duplications since we can get it from the struct Inner everywhere where It's needed.
Darksonn
reviewed
Aug 21, 2024
tokio/src/runtime/time/mod.rs
Outdated
Comment on lines
208
to
212
| let expiration_time = (0..rt_handle.time().inner.get_shard_size()) | ||
| .filter_map(|id| { | ||
| let wheel = wheels_lock.get_sharded_wheel_mut(id); | ||
| wheel.next_expiration_time() | ||
| }) |
There was a problem hiding this comment.
Could something like this make more sense?
wheels_lock.0.iter_mut()
.filter_map(|wheel| wheel.get_mut().next_expiration_time())
.min();
This way, we don't need to touch indexes at all.
There was a problem hiding this comment.
I like this. At least when calculating the min expiration time we indeed don't need the indexes anymore and we can get rid of one function.
Implemented it in 1610c12
| @@ -334,10 +352,16 @@ impl Handle { | |||
| if !waker_list.can_push() { | |||
| // Wake a batch of wakers. To avoid deadlock, we must do this with the lock temporarily dropped. | |||
| drop(lock); | |||
| drop(wheels_lock); | |||
There was a problem hiding this comment.
You also need to drop wheels_lock after the loop, since there's also a call to wake_all there.
There was a problem hiding this comment.
Sure! Missed that... Implemented in 1610c12.
Darksonn
approved these changes
Aug 27, 2024
kodiakhq bot
pushed a commit
to pdylanross/fatigue
that referenced
this pull request
Aug 30, 2024
Bumps tokio from 1.39.3 to 1.40.0.
Release notes
Sourced from tokio's releases.
Tokio v1.40.0
1.40.0 (August 30th, 2024)
Added
io: add util::SimplexStream (#6589)
process: stabilize Command::process_group (#6731)
sync: add {TrySendError,SendTimeoutError}::into_inner (#6755)
task: add JoinSet::join_all (#6784)
Added (unstable)
runtime: add Builder::{on_task_spawn, on_task_terminate} (#6742)
Changed
io: use vectored io for write_all_buf when possible (#6724)
runtime: prevent niche-optimization to avoid triggering miri (#6744)
sync: mark mpsc types as UnwindSafe (#6783)
sync,time: make Sleep and BatchSemaphore instrumentation explicit roots (#6727)
task: use NonZeroU64 for task::Id (#6733)
task: include panic message when printing JoinError (#6753)
task: add #[must_use] to JoinHandle::abort_handle (#6762)
time: eliminate timer wheel allocations (#6779)
Documented
docs: clarify that [build] section doesn't go in Cargo.toml (#6728)
io: clarify zero remaining capacity case (#6790)
macros: improve documentation for select! (#6774)
sync: document mpsc channel allocation behavior (#6773)
#6589: tokio-rs/tokio#6589
#6724: tokio-rs/tokio#6724
#6727: tokio-rs/tokio#6727
#6728: tokio-rs/tokio#6728
#6731: tokio-rs/tokio#6731
#6733: tokio-rs/tokio#6733
#6742: tokio-rs/tokio#6742
#6744: tokio-rs/tokio#6744
#6753: tokio-rs/tokio#6753
#6755: tokio-rs/tokio#6755
#6762: tokio-rs/tokio#6762
#6773: tokio-rs/tokio#6773
#6774: tokio-rs/tokio#6774
#6779: tokio-rs/tokio#6779
#6783: tokio-rs/tokio#6783
#6784: tokio-rs/tokio#6784
#6790: tokio-rs/tokio#6790
Commits
ea6d652 chore: prepare Tokio v1.40.0 (#6806)
11f66f4 chore: replace ready! with std::task::ready! (#6804)
479a56a time: eliminate timer wheel allocations (#6779)
b37f0de runtime: implement initial set of task hooks (#6742)
c9fad08 codec: fix typo in the docs for Encoder::Error (#6800)
cc70a21 task: add join_all method to JoinSet (#6784)
1ac8dff task: add AbortOnDropHandle type (#6786)
ff3f2a8 io: add SimplexStream (#6589)
5b9a290 io: clarify zero remaining capacity case (#6790)
70569bd task: fix typo in TaskTracker docs (#6792)
Additional commits viewable in compare view
Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
tglane
added a commit
to tglane/tokio
that referenced
this pull request
Aug 30, 2024
With tokio-rs#6779 we removed unnecessary allocations from the timerwheel by wrapping it in an `std::sync::RwLock`. Since the `Mutex` used in this part of the project uses an abstraction in `loom::sync::Mutex` to get rid of the poisoning aspects of `std::sync::Mutex` the same should probably be done for the used read-write lock struct. This commit introduces an abstraction to get rid of the poisoning aspects of `std::sync::RwLock` by introducing a wrapper to the `loom::sync` module similar to `loom::sync::Mutex`. Refs: tokio-rs#6779
mox692
pushed a commit
that referenced
this pull request
Sep 5, 2024
With #6779 we removed unnecessary allocations from the timerwheel by wrapping it in an `std::sync::RwLock`. Since the `Mutex` used in this part of the project uses an abstraction in `loom::sync::Mutex` to get rid of the poisoning aspects of `std::sync::Mutex` the same should probably be done for the used read-write lock struct. This commit introduces an abstraction to get rid of the poisoning aspects of `std::sync::RwLock` by introducing a wrapper to the `loom::sync` module similar to `loom::sync::Mutex`. Refs: #6779
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
#6683 introduced additional allocations to keep the locks of the timer wheel shards alive. #6757 is a request to get rid of those additional allocations. This PR should address this issue.
Solution
I wrapped the timer wheel shards in an
RwLock. With that we can get awrite-lock inpark_internalto prevent other threads from accessing the shards. In all other places theread-lock can be used for simultaneous access.Closes #6757