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

sync: use WakeList in Notify and batch_semaphore #4071

Merged
merged 4 commits into from
Aug 26, 2021
Merged

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Aug 25, 2021

Motivation

PR #4055 added a new WakeList type, to manage a potentially
uninitialized array when waking batches of wakers. This has the
advantage of not initializing a bunch of empty Options when only a
small number of tasks are being woken, potentially improving performance
in these cases.

Currently, WakeList is used only in the IO driver. However,
tokio::sync contains some code that's almost identical to the code in
the IO driver that was replaced with WakeList, so we can apply the
same optimizations there.

Solution

This branch changes tokio::sync::Notify and
tokio::sync::batch_semaphore::Semaphore to use WakeList when waking
batches of wakers. This was a pretty straightforward drop-in
replacement.

This commit updates `tokio::sync::Notify` to use the `WakeList` type
added in PR #4055. This may improve performance somewhat, as it will
avoid initializing a bunch of empty `Option`s when waking.

I'd like to make similar changes to `BatchSemaphore`, but this is a
somewhat larger change, as the wakers stored in the array are not
`Waker`s but an internal type, and permit assigning operations are
performed prior to waking.
This commit updates the internal semaphore implementation
(`batch_semaphore.rs`) to use the new `WakeList` type added in PR #4055.
@hawkw hawkw self-assigned this Aug 25, 2021
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Aug 25, 2021
Comment on lines 6 to 9
#[cfg(any(feature = "io-driver", feature = "sync"))]
mod wake_list;
#[cfg(any(feature = "io-driver", feature = "sync"))]
pub(crate) use wake_list::WakeList;
Copy link
Contributor

Choose a reason for hiding this comment

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

Using feature = "io-driver" is incorrect. See the macro definition:

macro_rules! cfg_io_driver {
($($item:item)*) => {
$(
#[cfg(any(
feature = "net",
feature = "process",
all(unix, feature = "signal"),
))]
#[cfg_attr(docsrs, doc(cfg(any(
feature = "net",
feature = "process",
all(unix, feature = "signal"),
))))]
$item
)*
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

agh, whoops --- thanks for catching that

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw merged commit 1e2e38b into master Aug 26, 2021
@hawkw hawkw deleted the eliza/wakelist-sync branch August 26, 2021 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-sync Module: tokio/sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants