-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Nesting Task
scopes
#4343
Nesting Task
scopes
#4343
Conversation
@@ -68,19 +68,22 @@ impl TaskPool { | |||
|
|||
let mut scope = Scope { | |||
executor, | |||
results: Vec::new(), | |||
results: Arc::new(Mutex::new(Vec::new())), |
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 think we want to avoid the use of Mutexes on the single threaded executor. The primary use case for the single threaded executor is for wasm and wasm doesn't support Atomic wait on the main thread which is what mutexes use when you build with atomics for wasm.
I think without enabling atomics, Mutexes become a no op, so this might work with a default wasm build config today.
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.
Right
I didn't implement it, but Joy came up with the idea of splitting scope
into a local variant and non-local variant.
Although originally we didn't decide on results
storage (I was pro-shared), if we split them up we can make a thread safe LocalScope
Signatures would look something like this:
LocalScope::spawn_local(FnOnce(&LocalScope, &Scope)); // keep both scopes
Scope::spawn(FnOnce(&Scope)); // keep scope, lose local scope
scopes like this:
struct LocalScope<T> {
local_executor: ...,
results: RefCell<Vec<T>>,
scope: &Scope,
}
struct Scope<T> {
executor: ...,
results: Mutex<Vec<T>>,
}
It's also worth mentioning that there were talks about moving wasm to multithreading although I'm not seeing any progress (nor contributing much :/)
@@ -265,7 +266,7 @@ impl Default for TaskPool { | |||
pub struct Scope<'scope, T> { | |||
executor: &'scope async_executor::Executor<'scope>, | |||
local_executor: &'scope async_executor::LocalExecutor<'scope>, | |||
spawned: Vec<async_executor::Task<T>>, | |||
spawned: Arc<Mutex<Vec<async_executor::Task<T>>>>, |
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.
Replacing this Arc with a reference to a Mutex<Vec<T>>
stored on the stack should be possible. Alternatively, a queue might also be better to have less locking overhead.
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.
Stack of which thread?
I've never seen a solution like that so I'm slightly confused
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'd be similar to how the code currently makes a fresh Executor
on the stack when scope
is called. We'd just also make a Mutex<Vec<Task<T>>>
on the stack, and store a regular rust reference to it inside Scope
Co-authored-by: MiniaczQ<MiniaczQ@gmail.com>
Co-authored-by: MiniaczQ<MiniaczQ@gmail.com>
Co-authored-by: MiniaczQ<MiniaczQ@gmail.com>
Co-authored-by: MiniaczQ <MiniaczQ@gmail.com>
Co-authored-by: MiniaczQ <MiniaczQ@gmail.com>
Co-authored-by: MiniaczQ <MiniaczQ@gmail.com>
Co-authored-by: MiniaczQ <MiniaczQ@gmail.com>
Co-authored-by: MiniaczQ <MiniaczQ@gmail.com>
Co-authored-by: MiniaczQ <MiniaczQ@gmail.com>
Co-authored-by: MiniaczQ <MiniaczQ@gmail.com>
Co-authored-by: MiniaczQ <MiniaczQ@gmail.com>
Co-authored-by: MiniaczQ <MiniaczQ@gmail.com>
Co-authored-by: MiniaczQ <MiniaczQ@gmail.com>
Co-authored-by: MiniaczQ <MiniaczQ@gmail.com>
Objective
attempt to
Allow for nested task scopes as in #4301.
Possibly not use a scope argument for further nesting.
Progress
I wrapped
Task
sresults
field inArc<Mutex<>>
to make it internally mutable,then changed functions signatures to no longer use
&mut
but&
.Currently stuck on
LocalExecutor
which is!Send + !Sync
, hence prevents nesting insideScope::spawn
.full error message
(nesting
spawn_local
works, but is stopped by another problem, I wanna focus on fixing the original)