rt: Remove `threaded_scheduler()` and `basic_scheduler()` #2876

LucioFranco · 2020-09-24T19:06:14Z

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 no rt_threaded, core_threads is ignored and will always use the basic scheduler
rt_core & rt_threaded, core_threads = 0 will use basic, core_threads = n, n > 0 will use threaded
no rt_core & no rt_threaded it will always use the shell runtime

There 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

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`.

LucioFranco · 2020-09-24T19:07:22Z

tokio-macros/src/entry.rs

+    // 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) };
+    // }


Don't really care if we merge this, likely going to be rewritten in a follow up PR anyways.

This doesn't include #2906, so yes, it will be rewritten by that PR if this is merged first.

LucioFranco · 2020-09-24T19:07:49Z

tokio/src/lib.rs

-        pub use tokio_macros::main;
-        pub use tokio_macros::test;
-    }
+    #[cfg(not(test))]


again here, this will be re-reviewed once I get to the macros. This Pr is just for the builder.

Darksonn · 2020-09-25T15:15:06Z

I have not yet looked at the code, but:

rt-core enabled and no rt-threaded, core_threads is ignored and will always use the basic scheduler

I really want to go away from situations where enabling features changes the behaviour of code that compiles without them. Can we have core_threads not exist at all when rt-threaded is disabled?

I'm also not convinced we should expose a runtime at all without rt-core. It makes sense to have one internally for e.g. blocking_recv, but I'd prefer if the shell runtime didn't exist at all from the user's point of view.

LucioFranco · 2020-09-25T15:16:45Z

I really want to go away from situations where enabling features changes the behaviour of code that compiles without them. Can we have core_threads not exist at all when rt-threaded is disabled?

I'm also not convinced we should expose a runtime at all without rt-core. It makes sense to have one internally for e.g. blocking_recv, but I'd prefer if the shell runtime didn't exist at all from the user's point of view.

I agree, but we could likely tackle this in a follow up PR? Could you post these thoughts also in #2720?

Darksonn

It seems reasonable.

tokio/src/runtime/mod.rs

carllerche

Left an inline question. I didn't have time to finish a full review yet.

carllerche · 2020-09-25T16:48:27Z

benches/spawn.rs

-        .threaded_scheduler()
-        .build()
-        .unwrap();
+    let runtime = tokio::runtime::Builder::new().build().unwrap();


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.

carllerche

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

carllerche · 2020-10-12T03:41:27Z

@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

carllerche · 2020-10-12T04:44:39Z

@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 👍.

carllerche · 2020-10-12T05:55:36Z

@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 rt-core feature flag works. Enabling "Hide whitespace changes" makes the diff a bit smaller.

Darksonn

I read through it and didn't notice any trouble. I haven't looked that closely at docs though.

LucioFranco

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?

LucioFranco · 2020-10-12T14:43:33Z

benches/signal.rs

        // Intentionally single threaded to measure delays in propagating wakes
-        .basic_scheduler()
+        .worker_threads(0)


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?

LucioFranco · 2020-10-12T14:43:50Z

benches/sync_semaphore.rs

-    let rt = tokio::runtime::Builder::new()
-        .basic_scheduler()
+    let rt = tokio::runtime::Builder::new_multi_thread()
+        .worker_threads(0)


same here is this valid?

tokio-macros/src/entry.rs

tokio-macros/src/lib.rs

tokio/src/coop.rs

LucioFranco · 2020-10-12T14:49:10Z

tokio/src/future/poll_fn.rs

@@ -1,3 +1,5 @@
+#![allow(dead_code)]


is this just dead in all cases? Should we cfg_attr this?

LucioFranco · 2020-10-12T14:51:37Z

tokio/src/runtime/builder.rs

-    Shell,
-    #[cfg(feature = "rt-core")]
-    Basic,
+pub(crate) enum Kind {


So no single thread runtime? or are we punting that?

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.

tokio/src/task/blocking.rs

LucioFranco · 2020-10-12T14:58:57Z

tokio/src/util/mod.rs

@@ -3,16 +3,26 @@ cfg_io_driver! {
    pub(crate) mod slab;
 }

+#[cfg(any(


lol this must have been fun

tokio-macros/src/entry.rs

Co-authored-by: Alice Ryhl <alice@ryhl.io>

…r-runtime-builder

stale

LucioFranco · 2020-10-12T17:08:10Z

Will be merging this once CI passes, I plan to do a full review of the docs once this has been merged.

carllerche · 2020-10-12T17:17:38Z

👍 merge this whenever CI passes.

rt: Remove threaded_scheduler() and basic_scheduler()

f3b8d88

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`.

LucioFranco requested review from carllerche, Darksonn and hawkw September 24, 2020 19:06

LucioFranco commented Sep 24, 2020

View reviewed changes

Fix docs

692b418

Darksonn added A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime C-enhancement Category: A PR with an enhancement or bugfix. labels Sep 25, 2020

Darksonn mentioned this pull request Sep 25, 2020

rt: Runtime refinements #2720

Closed

6 tasks

Darksonn approved these changes Sep 25, 2020

View reviewed changes

tokio/src/runtime/mod.rs Outdated Show resolved Hide resolved

carllerche reviewed Sep 25, 2020

View reviewed changes

Darksonn added 4 commits October 3, 2020 20:26

Rewrite tokio-macros to new argument format

1db81a9

Update tests

1ecd3bc

Rename to current_thread/threaded and remove max_threads

ccd426d

Use match in build

019945f

stsydow approved these changes Oct 6, 2020

View reviewed changes

Darksonn and others added 9 commits October 7, 2020 18:13

Fix tests

8c2b54a

rustfmt

a1ae97c

clippy

430993d

Try to fix doctests

10a2735

test_fail as main -> test

0775bd4

Add new_multi_thread/new_current_thread/new_single_thread

9c22b85

Add runtime builders

45c4977

remove shell runtime

2178252

Remove shell runtime test

97717ac

carllerche reviewed Oct 8, 2020

View reviewed changes

carllerche added 2 commits October 11, 2020 20:30

Merge remote-tracking branch 'origin/macro-new-builder' into lucio/re…

3b1eb17

…factor-runtime-builder

fix macro doc examples

61421e7

carllerche mentioned this pull request Oct 12, 2020

Update tokio-macros to new builder #2906

Closed

carllerche and others added 5 commits October 11, 2020 20:54

fix bad type

130beed

Update tokio/src/io/driver/mod.rs

2c9acd1

Co-authored-by: Alice Ryhl <alice@ryhl.io>

tests + rename num_workers

bc77b12

Merge branch 'lucio/refactor-runtime-builder' of https://github.com/t…

55ba939

…okio-rs/tokio into lucio/refactor-runtime-builder

tweak ff

9003409

carllerche added 6 commits October 11, 2020 21:50

fix a test

f8bb6a0

fix doc links

f10eedf

try to fix CI

5c5f205

tweak multi-threaded flavor

fb155dd

fix doc test

5a94b7e

update error message

22f5237

Darksonn approved these changes Oct 12, 2020

View reviewed changes

LucioFranco commented Oct 12, 2020

View reviewed changes

Darksonn reviewed Oct 12, 2020

View reviewed changes

tokio-macros/src/entry.rs Outdated Show resolved Hide resolved

LucioFranco and others added 3 commits October 12, 2020 12:48

Apply suggestions from code review

e3f2f86

Co-authored-by: Alice Ryhl <alice@ryhl.io>

address comments

3ad8f3b

Merge branch 'master' of github.com:tokio-rs/tokio into lucio/refacto…

5829241

…r-runtime-builder

LucioFranco merged commit 8880222 into master Oct 12, 2020

LucioFranco deleted the lucio/refactor-runtime-builder branch October 12, 2020 17:45

beanpuppy mentioned this pull request Oct 19, 2020

Errors while trying to access subreddit data halcyonnouveau/roux#18

Closed

nickelc mentioned this pull request Oct 24, 2020

docs: remove max_threads mentions in tokio-macros #3038

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rt: Remove `threaded_scheduler()` and `basic_scheduler()` #2876

rt: Remove `threaded_scheduler()` and `basic_scheduler()` #2876

LucioFranco commented Sep 24, 2020

LucioFranco Sep 24, 2020

Darksonn Oct 10, 2020

LucioFranco Sep 24, 2020

Darksonn commented Sep 25, 2020

LucioFranco commented Sep 25, 2020

Darksonn left a comment

carllerche left a comment

carllerche Sep 25, 2020

carllerche left a comment

carllerche commented Oct 12, 2020

carllerche commented Oct 12, 2020

carllerche commented Oct 12, 2020 •

edited

Darksonn left a comment

LucioFranco left a comment

LucioFranco Oct 12, 2020

LucioFranco Oct 12, 2020

LucioFranco Oct 12, 2020

LucioFranco Oct 12, 2020

carllerche Oct 12, 2020

LucioFranco Oct 12, 2020

LucioFranco commented Oct 12, 2020

carllerche commented Oct 12, 2020

rt: Remove threaded_scheduler() and basic_scheduler() #2876

rt: Remove threaded_scheduler() and basic_scheduler() #2876

Conversation

LucioFranco commented Sep 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darksonn commented Sep 25, 2020

LucioFranco commented Sep 25, 2020

Darksonn left a comment

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

carllerche commented Oct 12, 2020

carllerche commented Oct 12, 2020

carllerche commented Oct 12, 2020 • edited

Darksonn left a comment

Choose a reason for hiding this comment

LucioFranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucioFranco commented Oct 12, 2020

carllerche commented Oct 12, 2020

rt: Remove `threaded_scheduler()` and `basic_scheduler()` #2876

rt: Remove `threaded_scheduler()` and `basic_scheduler()` #2876

carllerche commented Oct 12, 2020 •

edited