From 92061467a3c4c0fdc3ea45d80c4bf5e971223f8e Mon Sep 17 00:00:00 2001 From: shuo Date: Sun, 26 Feb 2023 20:37:25 +0800 Subject: [PATCH 01/13] Fix asset_debug_server hang. There should be at most one ThreadExecutor's ticker for one thread. --- .../src/schedule/executor/multi_threaded.rs | 10 +++++-- crates/bevy_tasks/src/task_pool.rs | 19 ++++++++++--- crates/bevy_tasks/src/thread_executor.rs | 27 ++++++++++++++++--- 3 files changed, 47 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 16ab5f789675d..4d0dbfe2497cc 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -619,11 +619,17 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World } /// New-typed [`ThreadExecutor`] [`Resource`] that is used to run systems on the main thread -#[derive(Resource, Default, Clone)] +#[derive(Resource, Clone)] pub struct MainThreadExecutor(pub Arc>); +impl Default for MainThreadExecutor { + fn default() -> Self { + Self::new() + } +} + impl MainThreadExecutor { pub fn new() -> Self { - MainThreadExecutor(Arc::new(ThreadExecutor::new())) + MainThreadExecutor(TaskPool::get_thread_executor().clone()) } } diff --git a/crates/bevy_tasks/src/task_pool.rs b/crates/bevy_tasks/src/task_pool.rs index 4f37a8f34fbd4..9962200e08286 100644 --- a/crates/bevy_tasks/src/task_pool.rs +++ b/crates/bevy_tasks/src/task_pool.rs @@ -112,7 +112,12 @@ pub struct TaskPool { impl TaskPool { thread_local! { static LOCAL_EXECUTOR: async_executor::LocalExecutor<'static> = async_executor::LocalExecutor::new(); - static THREAD_EXECUTOR: ThreadExecutor<'static> = ThreadExecutor::new(); + static THREAD_EXECUTOR: Arc> = Arc::new(ThreadExecutor::new()); + } + + /// Each thread can only create one thread_executor, otherwise, there are good chances they will deadlock + pub fn get_thread_executor() -> Arc> { + Self::THREAD_EXECUTOR.with(|executor| executor.clone()) } /// Create a `TaskPool` with the default configuration. @@ -412,7 +417,11 @@ impl TaskPool { loop { let tick_forever = async { loop { - external_ticker.tick().or(scope_ticker.tick()).await; + if external_ticker.conflict_with(&scope_ticker) { + external_ticker.tick().await + } else { + external_ticker.tick().or(scope_ticker.tick()).await; + } } }; // we don't care if it errors. If a scoped task errors it will propagate @@ -436,7 +445,11 @@ impl TaskPool { loop { let tick_forever = async { loop { - external_ticker.tick().or(scope_ticker.tick()).await; + if external_ticker.conflict_with(&scope_ticker) { + external_ticker.tick().await; + } else { + external_ticker.tick().or(scope_ticker.tick()).await; + } } }; let _result = AssertUnwindSafe(tick_forever).catch_unwind().await.is_ok(); diff --git a/crates/bevy_tasks/src/thread_executor.rs b/crates/bevy_tasks/src/thread_executor.rs index 0ba66571db982..63ffb59f8d9b9 100644 --- a/crates/bevy_tasks/src/thread_executor.rs +++ b/crates/bevy_tasks/src/thread_executor.rs @@ -62,6 +62,19 @@ impl<'task> ThreadExecutor<'task> { Self::default() } + /// check whether same executor, if yes, then the `Ticker` should not + /// be used in same task. E.g, ticker_1.or(ticker_2).await. This will leak + /// the ticker and cause dead lock + pub fn is_same_executor(&self, other: &Self) -> bool { + if self.thread_id == other.thread_id { + // for same thread, we assert they are same object + assert!(std::ptr::eq(self, other)); + true + } else { + false + } + } + /// Spawn a task on the thread executor pub fn spawn( &self, @@ -77,7 +90,7 @@ impl<'task> ThreadExecutor<'task> { pub fn ticker<'ticker>(&'ticker self) -> Option> { if thread::current().id() == self.thread_id { return Some(ThreadExecutorTicker { - executor: &self.executor, + executor: self, _marker: PhantomData::default(), }); } @@ -90,20 +103,26 @@ impl<'task> ThreadExecutor<'task> { /// created on. #[derive(Debug)] pub struct ThreadExecutorTicker<'task, 'ticker> { - executor: &'ticker Executor<'task>, + executor: &'ticker ThreadExecutor<'task>, // make type not send or sync _marker: PhantomData<*const ()>, } impl<'task, 'ticker> ThreadExecutorTicker<'task, 'ticker> { + /// if executor is same, then the Ticker should not used + /// in same task + pub fn conflict_with(&self, other: &Self) -> bool { + self.executor.is_same_executor(other.executor) + } + /// Tick the thread executor. pub async fn tick(&self) { - self.executor.tick().await; + self.executor.executor.tick().await; } /// Synchronously try to tick a task on the executor. /// Returns false if if does not find a task to tick. pub fn try_tick(&self) -> bool { - self.executor.try_tick() + self.executor.executor.try_tick() } } From 3bc434c1ffcc20b1ccc271dcb8203e5798174b3a Mon Sep 17 00:00:00 2001 From: shuo Date: Sun, 26 Feb 2023 21:05:25 +0800 Subject: [PATCH 02/13] fix clippy --- crates/bevy_ecs/src/schedule/executor/multi_threaded.rs | 2 +- crates/bevy_tasks/src/task_pool.rs | 4 ++-- crates/bevy_tasks/src/thread_executor.rs | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 4d0dbfe2497cc..4a330890ecf01 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -630,6 +630,6 @@ impl Default for MainThreadExecutor { impl MainThreadExecutor { pub fn new() -> Self { - MainThreadExecutor(TaskPool::get_thread_executor().clone()) + MainThreadExecutor(TaskPool::get_thread_executor()) } } diff --git a/crates/bevy_tasks/src/task_pool.rs b/crates/bevy_tasks/src/task_pool.rs index 9962200e08286..f5aa295a18c7c 100644 --- a/crates/bevy_tasks/src/task_pool.rs +++ b/crates/bevy_tasks/src/task_pool.rs @@ -115,7 +115,7 @@ impl TaskPool { static THREAD_EXECUTOR: Arc> = Arc::new(ThreadExecutor::new()); } - /// Each thread can only create one thread_executor, otherwise, there are good chances they will deadlock + /// Each thread should only create one `ThreadExecutor`, otherwise, there are good chances they will deadlock pub fn get_thread_executor() -> Arc> { Self::THREAD_EXECUTOR.with(|executor| executor.clone()) } @@ -418,7 +418,7 @@ impl TaskPool { let tick_forever = async { loop { if external_ticker.conflict_with(&scope_ticker) { - external_ticker.tick().await + external_ticker.tick().await; } else { external_ticker.tick().or(scope_ticker.tick()).await; } diff --git a/crates/bevy_tasks/src/thread_executor.rs b/crates/bevy_tasks/src/thread_executor.rs index 63ffb59f8d9b9..397b440811040 100644 --- a/crates/bevy_tasks/src/thread_executor.rs +++ b/crates/bevy_tasks/src/thread_executor.rs @@ -63,11 +63,11 @@ impl<'task> ThreadExecutor<'task> { } /// check whether same executor, if yes, then the `Ticker` should not - /// be used in same task. E.g, ticker_1.or(ticker_2).await. This will leak + /// be used in same task. E.g, `ticker_1.or(ticker_2).await`. This will leak /// the ticker and cause dead lock pub fn is_same_executor(&self, other: &Self) -> bool { if self.thread_id == other.thread_id { - // for same thread, we assert they are same object + // for same thread, there should be only one instance assert!(std::ptr::eq(self, other)); true } else { From cb1929815d817297c892fd91c105b4abf3d41e11 Mon Sep 17 00:00:00 2001 From: shuo Date: Sun, 26 Feb 2023 21:32:49 +0800 Subject: [PATCH 03/13] fix wasm build --- crates/bevy_tasks/src/single_threaded_task_pool.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/bevy_tasks/src/single_threaded_task_pool.rs b/crates/bevy_tasks/src/single_threaded_task_pool.rs index 83e8084a6d3cd..b1546884bce51 100644 --- a/crates/bevy_tasks/src/single_threaded_task_pool.rs +++ b/crates/bevy_tasks/src/single_threaded_task_pool.rs @@ -56,6 +56,11 @@ impl TaskPoolBuilder { pub struct TaskPool {} impl TaskPool { + /// Just create a new `ThreadExecutor` for wasm + pub fn get_thread_executor() -> Arc> { + Arc::new(ThreadExecutor::new()) + } + /// Create a `TaskPool` with the default configuration. pub fn new() -> Self { TaskPoolBuilder::new().build() From f15ac4fee816af2f8f86bce4869b303a3ad425d4 Mon Sep 17 00:00:00 2001 From: shuo Date: Mon, 27 Feb 2023 08:33:51 +0800 Subject: [PATCH 04/13] move ticker conflict check out of loop --- crates/bevy_tasks/src/task_pool.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/crates/bevy_tasks/src/task_pool.rs b/crates/bevy_tasks/src/task_pool.rs index f5aa295a18c7c..d19183f3fb56f 100644 --- a/crates/bevy_tasks/src/task_pool.rs +++ b/crates/bevy_tasks/src/task_pool.rs @@ -416,10 +416,12 @@ impl TaskPool { let execute_forever = async move { loop { let tick_forever = async { - loop { - if external_ticker.conflict_with(&scope_ticker) { + if external_ticker.conflict_with(&scope_ticker) { + loop { external_ticker.tick().await; - } else { + } + } else { + loop { external_ticker.tick().or(scope_ticker.tick()).await; } } @@ -444,10 +446,12 @@ impl TaskPool { let execute_forever = async { loop { let tick_forever = async { - loop { - if external_ticker.conflict_with(&scope_ticker) { + if external_ticker.conflict_with(&scope_ticker) { + loop { external_ticker.tick().await; - } else { + } + } else { + loop { external_ticker.tick().or(scope_ticker.tick()).await; } } From 6d22cc51b447b1105bff60ad815ebde0d519b8ea Mon Sep 17 00:00:00 2001 From: shuo Date: Mon, 27 Feb 2023 15:15:35 +0800 Subject: [PATCH 05/13] Add nested app example to show ThreadExecutor deadlock. Disable the conflict check, it would block. --- Cargo.toml | 10 ++++++++++ examples/app/nested_app.rs | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+) create mode 100644 examples/app/nested_app.rs diff --git a/Cargo.toml b/Cargo.toml index 0b1427689d214..e611be4ec2a7f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -687,6 +687,16 @@ description = "Demonstrates the creation and registration of a custom plugin" category = "Application" wasm = true +[[example]] +name = "nested_app" +path = "examples/app/nested_app.rs" + +[package.metadata.example.nested_app] +name = "Nested" +description = "Demonstrates the creation of nested app" +category = "Application" +wasm = false + [[example]] name = "plugin_group" path = "examples/app/plugin_group.rs" diff --git a/examples/app/nested_app.rs b/examples/app/nested_app.rs new file mode 100644 index 0000000000000..8dd99a11556a9 --- /dev/null +++ b/examples/app/nested_app.rs @@ -0,0 +1,19 @@ +use bevy::prelude::*; + +fn run_sub_app(mut sub_app: NonSendMut) { + sub_app.app.update(); +} + +struct DebugApp { + app: App, +} + +fn main() { + let mut app = App::new(); + + let sub_app = App::new(); + app.insert_non_send_resource(DebugApp { app: sub_app }); + app.add_system(run_sub_app); + + app.update(); +} From 6d45a1853169fa8a519f6d1a6738b30fe113cee3 Mon Sep 17 00:00:00 2001 From: shuo Date: Mon, 27 Feb 2023 15:21:51 +0800 Subject: [PATCH 06/13] fix ci --- examples/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/examples/README.md b/examples/README.md index c54ca491ec420..f1b62cd37e47d 100644 --- a/examples/README.md +++ b/examples/README.md @@ -153,6 +153,7 @@ Example | Description [Empty with Defaults](../examples/app/empty_defaults.rs) | An empty application with default plugins [Headless](../examples/app/headless.rs) | An application that runs without default plugins [Logs](../examples/app/logs.rs) | Illustrate how to use generate log output +[Nested](../examples/app/nested_app.rs) | Demonstrates the creation of nested app [No Renderer](../examples/app/no_renderer.rs) | An application that runs with default plugins and displays an empty window, but without an actual renderer [Plugin](../examples/app/plugin.rs) | Demonstrates the creation and registration of a custom plugin [Plugin Group](../examples/app/plugin_group.rs) | Demonstrates the creation and registration of a custom plugin group From f6b7b4538fa3d2209b69aaafc3513c5bfecc36d9 Mon Sep 17 00:00:00 2001 From: shuo Date: Tue, 28 Feb 2023 20:40:56 +0800 Subject: [PATCH 07/13] clean code for review --- Cargo.toml | 10 ------- crates/bevy_tasks/src/task_pool.rs | 20 +++---------- crates/bevy_tasks/src/thread_executor.rs | 36 +++++++++++------------- examples/app/nested_app.rs | 19 ------------- 4 files changed, 20 insertions(+), 65 deletions(-) delete mode 100644 examples/app/nested_app.rs diff --git a/Cargo.toml b/Cargo.toml index e611be4ec2a7f..0b1427689d214 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -687,16 +687,6 @@ description = "Demonstrates the creation and registration of a custom plugin" category = "Application" wasm = true -[[example]] -name = "nested_app" -path = "examples/app/nested_app.rs" - -[package.metadata.example.nested_app] -name = "Nested" -description = "Demonstrates the creation of nested app" -category = "Application" -wasm = false - [[example]] name = "plugin_group" path = "examples/app/plugin_group.rs" diff --git a/crates/bevy_tasks/src/task_pool.rs b/crates/bevy_tasks/src/task_pool.rs index d19183f3fb56f..1630e976edc19 100644 --- a/crates/bevy_tasks/src/task_pool.rs +++ b/crates/bevy_tasks/src/task_pool.rs @@ -416,14 +416,8 @@ impl TaskPool { let execute_forever = async move { loop { let tick_forever = async { - if external_ticker.conflict_with(&scope_ticker) { - loop { - external_ticker.tick().await; - } - } else { - loop { - external_ticker.tick().or(scope_ticker.tick()).await; - } + loop { + external_ticker.or_tick(&scope_ticker).await; } }; // we don't care if it errors. If a scoped task errors it will propagate @@ -446,14 +440,8 @@ impl TaskPool { let execute_forever = async { loop { let tick_forever = async { - if external_ticker.conflict_with(&scope_ticker) { - loop { - external_ticker.tick().await; - } - } else { - loop { - external_ticker.tick().or(scope_ticker.tick()).await; - } + loop { + external_ticker.or_tick(&scope_ticker).await; } }; let _result = AssertUnwindSafe(tick_forever).catch_unwind().await.is_ok(); diff --git a/crates/bevy_tasks/src/thread_executor.rs b/crates/bevy_tasks/src/thread_executor.rs index 397b440811040..f87c6c978e541 100644 --- a/crates/bevy_tasks/src/thread_executor.rs +++ b/crates/bevy_tasks/src/thread_executor.rs @@ -4,7 +4,7 @@ use std::{ }; use async_executor::{Executor, Task}; -use futures_lite::Future; +use futures_lite::{Future, FutureExt}; /// An executor that can only be ticked on the thread it was instantiated on. But /// can spawn `Send` tasks from other threads. @@ -62,19 +62,6 @@ impl<'task> ThreadExecutor<'task> { Self::default() } - /// check whether same executor, if yes, then the `Ticker` should not - /// be used in same task. E.g, `ticker_1.or(ticker_2).await`. This will leak - /// the ticker and cause dead lock - pub fn is_same_executor(&self, other: &Self) -> bool { - if self.thread_id == other.thread_id { - // for same thread, there should be only one instance - assert!(std::ptr::eq(self, other)); - true - } else { - false - } - } - /// Spawn a task on the thread executor pub fn spawn( &self, @@ -96,6 +83,11 @@ impl<'task> ThreadExecutor<'task> { } None } + + /// check whether `self` and `other` is the same executor + fn is_same_executor(&self, other: &Self) -> bool { + std::ptr::eq(self, other) + } } /// Used to tick the [`ThreadExecutor`]. The executor does not @@ -108,12 +100,6 @@ pub struct ThreadExecutorTicker<'task, 'ticker> { _marker: PhantomData<*const ()>, } impl<'task, 'ticker> ThreadExecutorTicker<'task, 'ticker> { - /// if executor is same, then the Ticker should not used - /// in same task - pub fn conflict_with(&self, other: &Self) -> bool { - self.executor.is_same_executor(other.executor) - } - /// Tick the thread executor. pub async fn tick(&self) { self.executor.executor.tick().await; @@ -124,6 +110,16 @@ impl<'task, 'ticker> ThreadExecutorTicker<'task, 'ticker> { pub fn try_tick(&self) -> bool { self.executor.executor.try_tick() } + + /// join tick `self` and `other` with `or` + /// ref: https://github.com/bevyengine/bevy/pull/7825 + pub async fn or_tick(&self, other: &Self) { + if self.executor.is_same_executor(other.executor) { + self.tick().await + } else { + self.tick().or(other.tick()).await + } + } } #[cfg(test)] diff --git a/examples/app/nested_app.rs b/examples/app/nested_app.rs deleted file mode 100644 index 8dd99a11556a9..0000000000000 --- a/examples/app/nested_app.rs +++ /dev/null @@ -1,19 +0,0 @@ -use bevy::prelude::*; - -fn run_sub_app(mut sub_app: NonSendMut) { - sub_app.app.update(); -} - -struct DebugApp { - app: App, -} - -fn main() { - let mut app = App::new(); - - let sub_app = App::new(); - app.insert_non_send_resource(DebugApp { app: sub_app }); - app.add_system(run_sub_app); - - app.update(); -} From 46bf6e6514756e4b4a3c8491bf8e3dd10b80e8b0 Mon Sep 17 00:00:00 2001 From: shuo Date: Tue, 28 Feb 2023 20:46:21 +0800 Subject: [PATCH 08/13] fix ci --- crates/bevy_tasks/src/thread_executor.rs | 8 ++++---- examples/README.md | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/crates/bevy_tasks/src/thread_executor.rs b/crates/bevy_tasks/src/thread_executor.rs index f87c6c978e541..95bb5ce611f39 100644 --- a/crates/bevy_tasks/src/thread_executor.rs +++ b/crates/bevy_tasks/src/thread_executor.rs @@ -111,13 +111,13 @@ impl<'task, 'ticker> ThreadExecutorTicker<'task, 'ticker> { self.executor.executor.try_tick() } - /// join tick `self` and `other` with `or` - /// ref: https://github.com/bevyengine/bevy/pull/7825 + /// join tick `self` and `other` with `or`. + /// check [this pr](https://github.com/bevyengine/bevy/pull/7825) for reference pub async fn or_tick(&self, other: &Self) { if self.executor.is_same_executor(other.executor) { - self.tick().await + self.tick().await; } else { - self.tick().or(other.tick()).await + self.tick().or(other.tick()).await; } } } diff --git a/examples/README.md b/examples/README.md index f1b62cd37e47d..c54ca491ec420 100644 --- a/examples/README.md +++ b/examples/README.md @@ -153,7 +153,6 @@ Example | Description [Empty with Defaults](../examples/app/empty_defaults.rs) | An empty application with default plugins [Headless](../examples/app/headless.rs) | An application that runs without default plugins [Logs](../examples/app/logs.rs) | Illustrate how to use generate log output -[Nested](../examples/app/nested_app.rs) | Demonstrates the creation of nested app [No Renderer](../examples/app/no_renderer.rs) | An application that runs with default plugins and displays an empty window, but without an actual renderer [Plugin](../examples/app/plugin.rs) | Demonstrates the creation and registration of a custom plugin [Plugin Group](../examples/app/plugin_group.rs) | Demonstrates the creation and registration of a custom plugin group From 4f9af7f80f4a44a69067bb79a4228115a6ae3d1c Mon Sep 17 00:00:00 2001 From: shuo Date: Wed, 1 Mar 2023 10:16:52 +0800 Subject: [PATCH 09/13] add `same executor checking` to existing executor branching code --- crates/bevy_tasks/src/task_pool.rs | 24 +++++++++++++++--------- crates/bevy_tasks/src/thread_executor.rs | 10 +++------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/crates/bevy_tasks/src/task_pool.rs b/crates/bevy_tasks/src/task_pool.rs index 1630e976edc19..b8f4131b89af7 100644 --- a/crates/bevy_tasks/src/task_pool.rs +++ b/crates/bevy_tasks/src/task_pool.rs @@ -382,8 +382,10 @@ impl TaskPool { // we get this from a thread local so we should always be on the scope executors thread. let scope_ticker = scope_executor.ticker().unwrap(); - if let Some(external_ticker) = external_executor.ticker() { - if tick_task_pool_executor { + match (external_executor.ticker(), tick_task_pool_executor) { + (Some(external_ticker), true) + if !external_ticker.is_same_executor(&scope_ticker) => + { Self::execute_global_external_scope( executor, external_ticker, @@ -391,14 +393,18 @@ impl TaskPool { get_results, ) .await - } else { + } + (Some(external_ticker), false) + if !external_ticker.is_same_executor(&scope_ticker) => + { Self::execute_external_scope(external_ticker, scope_ticker, get_results) .await } - } else if tick_task_pool_executor { - Self::execute_global_scope(executor, scope_ticker, get_results).await - } else { - Self::execute_scope(scope_ticker, get_results).await + // either external_executor is none or it is same as scope_executor + (_, true) => { + Self::execute_global_scope(executor, scope_ticker, get_results).await + } + (_, false) => Self::execute_scope(scope_ticker, get_results).await, } }) } @@ -417,7 +423,7 @@ impl TaskPool { loop { let tick_forever = async { loop { - external_ticker.or_tick(&scope_ticker).await; + external_ticker.tick().or(scope_ticker.tick()).await; } }; // we don't care if it errors. If a scoped task errors it will propagate @@ -441,7 +447,7 @@ impl TaskPool { loop { let tick_forever = async { loop { - external_ticker.or_tick(&scope_ticker).await; + external_ticker.tick().or(scope_ticker.tick()).await; } }; let _result = AssertUnwindSafe(tick_forever).catch_unwind().await.is_ok(); diff --git a/crates/bevy_tasks/src/thread_executor.rs b/crates/bevy_tasks/src/thread_executor.rs index 95bb5ce611f39..5f21c801f690a 100644 --- a/crates/bevy_tasks/src/thread_executor.rs +++ b/crates/bevy_tasks/src/thread_executor.rs @@ -111,14 +111,10 @@ impl<'task, 'ticker> ThreadExecutorTicker<'task, 'ticker> { self.executor.executor.try_tick() } - /// join tick `self` and `other` with `or`. + /// Returns true if `self` and `other`'s executor is same /// check [this pr](https://github.com/bevyengine/bevy/pull/7825) for reference - pub async fn or_tick(&self, other: &Self) { - if self.executor.is_same_executor(other.executor) { - self.tick().await; - } else { - self.tick().or(other.tick()).await; - } + pub fn is_same_executor(&self, other: &Self) -> bool { + std::ptr::eq(self.executor, other.executor) } } From 45beaba764f2f9233aa398617391e3d84ebc82af Mon Sep 17 00:00:00 2001 From: shuo Date: Wed, 1 Mar 2023 10:27:41 +0800 Subject: [PATCH 10/13] remove unused code --- crates/bevy_tasks/src/thread_executor.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/crates/bevy_tasks/src/thread_executor.rs b/crates/bevy_tasks/src/thread_executor.rs index 5f21c801f690a..1df9edf0468a6 100644 --- a/crates/bevy_tasks/src/thread_executor.rs +++ b/crates/bevy_tasks/src/thread_executor.rs @@ -4,7 +4,7 @@ use std::{ }; use async_executor::{Executor, Task}; -use futures_lite::{Future, FutureExt}; +use futures_lite::Future; /// An executor that can only be ticked on the thread it was instantiated on. But /// can spawn `Send` tasks from other threads. @@ -83,11 +83,6 @@ impl<'task> ThreadExecutor<'task> { } None } - - /// check whether `self` and `other` is the same executor - fn is_same_executor(&self, other: &Self) -> bool { - std::ptr::eq(self, other) - } } /// Used to tick the [`ThreadExecutor`]. The executor does not From c2bd15866496e65eb6a1b29c364ff69da6f0a5c5 Mon Sep 17 00:00:00 2001 From: shuo Date: Thu, 2 Mar 2023 09:34:18 +0800 Subject: [PATCH 11/13] fix comment --- crates/bevy_tasks/src/task_pool.rs | 2 ++ crates/bevy_tasks/src/thread_executor.rs | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_tasks/src/task_pool.rs b/crates/bevy_tasks/src/task_pool.rs index b8f4131b89af7..247a8faeb93d3 100644 --- a/crates/bevy_tasks/src/task_pool.rs +++ b/crates/bevy_tasks/src/task_pool.rs @@ -381,6 +381,8 @@ impl TaskPool { let tick_task_pool_executor = tick_task_pool_executor || self.threads.is_empty(); // we get this from a thread local so we should always be on the scope executors thread. + // note: it is possible `scope_executor` and `external_executor` is the same executor, + // in that case, we should only tick one of them, otherwise, it may cause deadlock. let scope_ticker = scope_executor.ticker().unwrap(); match (external_executor.ticker(), tick_task_pool_executor) { (Some(external_ticker), true) diff --git a/crates/bevy_tasks/src/thread_executor.rs b/crates/bevy_tasks/src/thread_executor.rs index 1df9edf0468a6..9ec860bb0fb43 100644 --- a/crates/bevy_tasks/src/thread_executor.rs +++ b/crates/bevy_tasks/src/thread_executor.rs @@ -107,7 +107,6 @@ impl<'task, 'ticker> ThreadExecutorTicker<'task, 'ticker> { } /// Returns true if `self` and `other`'s executor is same - /// check [this pr](https://github.com/bevyengine/bevy/pull/7825) for reference pub fn is_same_executor(&self, other: &Self) -> bool { std::ptr::eq(self.executor, other.executor) } From 90e1d0bf319fbac264bbfbe7a7f19763add1acef Mon Sep 17 00:00:00 2001 From: shuo Date: Thu, 2 Mar 2023 14:26:02 +0800 Subject: [PATCH 12/13] remove default branch from match --- crates/bevy_tasks/src/task_pool.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/bevy_tasks/src/task_pool.rs b/crates/bevy_tasks/src/task_pool.rs index 247a8faeb93d3..e52dd744d7c07 100644 --- a/crates/bevy_tasks/src/task_pool.rs +++ b/crates/bevy_tasks/src/task_pool.rs @@ -384,10 +384,12 @@ impl TaskPool { // note: it is possible `scope_executor` and `external_executor` is the same executor, // in that case, we should only tick one of them, otherwise, it may cause deadlock. let scope_ticker = scope_executor.ticker().unwrap(); - match (external_executor.ticker(), tick_task_pool_executor) { - (Some(external_ticker), true) - if !external_ticker.is_same_executor(&scope_ticker) => - { + let external_ticker = external_executor + .ticker() + .filter(|external_ticker| !external_ticker.is_same_executor(&scope_ticker)); + + match (external_ticker, tick_task_pool_executor) { + (Some(external_ticker), true) => { Self::execute_global_external_scope( executor, external_ticker, @@ -396,17 +398,15 @@ impl TaskPool { ) .await } - (Some(external_ticker), false) - if !external_ticker.is_same_executor(&scope_ticker) => - { + (Some(external_ticker), false) => { Self::execute_external_scope(external_ticker, scope_ticker, get_results) .await } // either external_executor is none or it is same as scope_executor - (_, true) => { + (None, true) => { Self::execute_global_scope(executor, scope_ticker, get_results).await } - (_, false) => Self::execute_scope(scope_ticker, get_results).await, + (None, false) => Self::execute_scope(scope_ticker, get_results).await, } }) } From 9312549c89c47a88e60a529fc2bc7c2bf28b1429 Mon Sep 17 00:00:00 2001 From: shuo Date: Thu, 2 Mar 2023 14:29:17 +0800 Subject: [PATCH 13/13] refine code a bit --- crates/bevy_tasks/src/task_pool.rs | 8 +++++--- crates/bevy_tasks/src/thread_executor.rs | 10 +++++----- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/crates/bevy_tasks/src/task_pool.rs b/crates/bevy_tasks/src/task_pool.rs index e52dd744d7c07..f66d1cd615d52 100644 --- a/crates/bevy_tasks/src/task_pool.rs +++ b/crates/bevy_tasks/src/task_pool.rs @@ -384,9 +384,11 @@ impl TaskPool { // note: it is possible `scope_executor` and `external_executor` is the same executor, // in that case, we should only tick one of them, otherwise, it may cause deadlock. let scope_ticker = scope_executor.ticker().unwrap(); - let external_ticker = external_executor - .ticker() - .filter(|external_ticker| !external_ticker.is_same_executor(&scope_ticker)); + let external_ticker = if !external_executor.is_same(scope_executor) { + external_executor.ticker() + } else { + None + }; match (external_ticker, tick_task_pool_executor) { (Some(external_ticker), true) => { diff --git a/crates/bevy_tasks/src/thread_executor.rs b/crates/bevy_tasks/src/thread_executor.rs index 9ec860bb0fb43..7495fe525e052 100644 --- a/crates/bevy_tasks/src/thread_executor.rs +++ b/crates/bevy_tasks/src/thread_executor.rs @@ -83,6 +83,11 @@ impl<'task> ThreadExecutor<'task> { } None } + + /// Returns true if `self` and `other`'s executor is same + pub fn is_same(&self, other: &Self) -> bool { + std::ptr::eq(self, other) + } } /// Used to tick the [`ThreadExecutor`]. The executor does not @@ -105,11 +110,6 @@ impl<'task, 'ticker> ThreadExecutorTicker<'task, 'ticker> { pub fn try_tick(&self) -> bool { self.executor.executor.try_tick() } - - /// Returns true if `self` and `other`'s executor is same - pub fn is_same_executor(&self, other: &Self) -> bool { - std::ptr::eq(self.executor, other.executor) - } } #[cfg(test)]