-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
rt: Remove threaded_scheduler()
and basic_scheduler()
#2876
Conversation
This PR removes the ability to set the threaded_scheduler and basic_scheduler directly via the builder methods. Now to select the basic_scheduler you must use `core_threads`.
tokio-macros/src/entry.rs
Outdated
// if let Some(v) = core_threads { | ||
// rt = quote! { #rt.core_threads(#v) }; | ||
// } | ||
|
||
// if let Some(v) = max_threads { | ||
// rt = quote! { #rt.max_threads(#v) }; | ||
// } | ||
|
||
if is_test { | ||
rt = quote! { #rt.core_threads(0) }; | ||
} | ||
|
||
// if is_test && core_threads.is_none() { | ||
// rt = quote! { #rt.core_threads(0) }; | ||
// } |
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.
Don't really care if we merge this, likely going to be rewritten in a follow up PR anyways.
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.
This doesn't include #2906, so yes, it will be rewritten by that PR if this is merged first.
tokio/src/lib.rs
Outdated
pub use tokio_macros::main; | ||
pub use tokio_macros::test; | ||
} | ||
#[cfg(not(test))] |
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.
again here, this will be re-reviewed once I get to the macros. This Pr is just for the builder.
I have not yet looked at the code, but:
I really want to go away from situations where enabling features changes the behaviour of code that compiles without them. Can we have I'm also not convinced we should expose a runtime at all without |
I agree, but we could likely tackle this in a follow up PR? Could you post these thoughts also in #2720? |
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.
It seems reasonable.
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.
Left an inline question. I didn't have time to finish a full review yet.
benches/spawn.rs
Outdated
.threaded_scheduler() | ||
.build() | ||
.unwrap(); | ||
let runtime = tokio::runtime::Builder::new().build().unwrap(); |
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.
In theory, this shouldn't work. A default builder should not enable the work-stealing scheduler. I would expect this to panic or enable the shell runtime.
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.
Looks like it is on track 👍 I didn't look at the mcro changes yet.
If it makes things easier, you can punt entirely on new_single_thread
for this PR.
…factor-runtime-builder
@Darksonn You can configure the diff viewer to disable whitespace changes. That will hide any changes in indentation from the diff. |
Co-authored-by: Alice Ryhl <alice@ryhl.io>
…okio-rs/tokio into lucio/refactor-runtime-builder
@davidbarsky I think your doc comments are good, but they aren't in scope for this PR. I would just merge them, but the comments are out of date. I'd suggest you open your own PR to submit those tweaks 👍. |
@Darksonn @LucioFranco Ok, this PR should be good to go. I merged in @Darksonn's macro PR and updated it to match the Builder names. #[tokio::main(flavor = "multi_thread", worker_threads = "4")] The change got a bit large. This is mostly due to the changes in how the |
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.
I read through it and didn't notice any trouble. I haven't looked that closely at docs though.
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.
I can't approve my own PR but left some comments. I am assuming single thread is gone we will add this back in a follow up or is it gone forever?
// Intentionally single threaded to measure delays in propagating wakes | ||
.basic_scheduler() | ||
.worker_threads(0) |
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.
This to me doesn't make sense, why 0? I would imagine we should not allow that unless the multi thread runtime can be a current thread one?
let rt = tokio::runtime::Builder::new() | ||
.basic_scheduler() | ||
let rt = tokio::runtime::Builder::new_multi_thread() | ||
.worker_threads(0) |
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.
same here is this valid?
@@ -1,3 +1,5 @@ | |||
#![allow(dead_code)] |
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.
is this just dead in all cases? Should we cfg_attr
this?
Shell, | ||
#[cfg(feature = "rt-core")] | ||
Basic, | ||
pub(crate) enum Kind { |
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.
So no single thread runtime? or are we punting that?
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.
No single thread for now IMO. We can do that post. It's a lot easier to do a single-thread by hand now as you can throw Runtime
in an arc.
@@ -3,16 +3,26 @@ cfg_io_driver! { | |||
pub(crate) mod slab; | |||
} | |||
|
|||
#[cfg(any( |
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.
lol this must have been fun
Co-authored-by: Alice Ryhl <alice@ryhl.io>
…r-runtime-builder
Will be merging this once CI passes, I plan to do a full review of the docs once this has been merged. |
👍 merge this whenever CI passes. |
This PR removes the ability to set the threaded_scheduler
and basic_scheduler directly via the builder methods. Now
to select the basic_scheduler you must use
core_threads
.For now:
rt_core
enabled and nort_threaded
, core_threads is ignored and will always use the basic schedulerrt_core
&rt_threaded
,core_threads = 0
will use basic,core_threads = n, n > 0
will use threadedrt_core
& nort_threaded
it will always use the shell runtimeThere are two new follow up issues in #2720, one for running the basic scheduler on a bg threaded instead of a single threaded threaded runtime and a re-review of the docs.
Alot of the macros have also been disabled to allow tests to pass, I expect to follow up on this and nix a lot of our macro code to simplify it in a follow up PR.
Related issue #2720