Skip to content

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Nov 16, 2022

Objective

Solution

This PR implements a proof-of-concept for "coroutine systems" defined using async. Because it is not sound to hold a reference to ECS resources over an await point, access to the system parameters is limited to during a sync closure through a co_with! macro.

This abuses the fact that macro names are not hygienic to define the co_with! macro used to access system parameters. This scheme allows minimizing the annotation overhead required of the user, importantly allowing full elision of lifetimes just as with normal function systems.

I did attempt to skip the need to re-name the system parameters when entering co_with! (essentially getting the "magic mutation" syntax for yield closures), but unfortunately for this use case, naming the parameters within the closure always refers to the outer names, not the newly bound names due to the new semitransparent macro_rules! layer.

The macro techniques to do so
diff --git a/crates/bevy_async/src/lib.rs b/crates/bevy_async/src/lib.rs
index 5fbeae7f..166a3a76 100644
--- a/crates/bevy_async/src/lib.rs
+++ b/crates/bevy_async/src/lib.rs
@@ -53,14 +53,14 @@
 //!     co!(async move |state: Local<usize>| loop {
 //!         let name = String::from("bevy_async");
 //!         // Access the parameters by using co_with!
-//!         let _ = co_with!(|state| {
+//!         let _ = co_with! {
 //!             // Within this closure, the parameters are available.
 //!             println!("The local's current value is {}", *state);
 //!             // The closure borrows normally from the containing scope.
 //!             println!("The name is {}", name);
 //!             // The closure can pass state back to the async context.
 //!             (&*name, *state)
-//!         });
+//!         };
 //!         // Outside co_with! is an async context where you can use await.
 //!         // It's best practice to spawn async work onto a task pool
 //!         // and await the task handle to minimize redundant polling.
@@ -370,8 +370,12 @@ macro_rules! co {
                     let ($($arg,)*) = unsafe { co.fetch_params_unchecked() };
                     f($($arg,)*)
                 }
-                macro_rules! co_with {($with:expr) => {
-                    co_with(&mut co, $with)
+                $crate::__item_with_dollar! {($_:tt) => {
+                    macro_rules! co_with {($_($_ with:tt)*) => {
+                        co_with(&mut co, |$($arg),*| {
+                            $_($_ with)*
+                        })
+                    }}
                 }}
                 loop {
                     $body;
@@ -383,3 +387,10 @@ macro_rules! co {
         )
     };
 }
+
+#[doc(hidden)]
+#[macro_export]
+macro_rules! __item_with_dollar {(($($in:tt)*) => {$($out:tt)*}) => {
+    macro_rules! __emit {($($in)*) => {$($out)*}}
+    __emit! {$}
+}}
error[E0381]: used binding `state` isn't initialized
  --> src\lib.rs:56:17
   |
9  |  /     co!(async move |state: Local<usize>| loop {
10 |  |         let name = String::from("bevy_async");
11 |  |         // Access the parameters by using co_with!
12 |  |         let _ = co_with! {
   |  |_________________^
13 | ||             // Within this closure, the parameters are available.
14 | ||             println!("The local's current value is {}", *state);
   | ||                                                          ----- borrow occurs due to use in closure
15 | ||             // The closure borrows normally from the containing scope.
...  ||
18 | ||             (&*name, *state)
19 | ||         };
   | ||_________^ `state` used here but it isn't initialized
...   |
26 |  |         }).await;
27 |  |     })
   |  |______- binding declared here but left uninitialized
   |
   = note: this error originates in the macro `co_with` (in Nightly builds, run with -Z macro-backtrace for more info)

The reason this doesn't work seems to be that the co_with! definition introduces a fresh scope which the expanded idents are attached to, rather than the intent in this case of keeping their originally captured context. I've tried a number of permutations but was unable to get the desired behavior here.


This can mostly be implemented out-of-tree, but requires making a few more things accessible from bevy_ecs to be able to implement System properly.

@CAD97
Copy link
Contributor Author

CAD97 commented Nov 16, 2022

cc @hanabi1224, if you're interested.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR C-Feature A new feature, making something new possible labels Nov 16, 2022
Copy link
Contributor

@jakobhellermann jakobhellermann left a comment

Choose a reason for hiding this comment

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

Very interesting.

I think if we can get them to work, the most useful async/generator system would be ones replacing a manual state machine such as this:

#[derive(Default)]
enum State {
    #[default]
    Initial,
    Loading(Handle<Image>),
    Loaded(Handle<Image>),
    Done,
}

fn run(
    mut state: Local<State>,
    asset_server: ResMut<AssetServer>,
    mut images: ResMut<Assets<Image>>,
) {
    match &*state {
        State::Initial => {
            let handle = asset_server.load("branding/icon.png");
            *state = State::Loading(handle);
        }
        State::Loading(handle) => match asset_server.get_load_state(handle) {
            LoadState::Loading => return,
            LoadState::Loaded => *state = State::Loaded(handle.clone()),
            _ => todo!(),
        },
        State::Loaded(handle) => {
            let image = images.get_mut(&handle).unwrap();
            image.sampler_descriptor = ImageSampler::linear();
            *state = State::Done;
        }
        State::Done => {}
    };
}

I tried writing this system with the co! macro and the end result looked like this:

    co!(
        async move |asset_server: ResMut<AssetServer>, assets: ResMut<Assets<Image>>| loop {
            let handle: Handle<Image> =
                co_with!(|asset_server, _| asset_server.load("branding/icon.png"));

            loop {
                let load_state = co_with!(|server, _| server.get_load_state(&handle));
                match load_state {
                    LoadState::Loaded => break,
                    LoadState::Loading => yield_now().await,
                    _ => todo!(),
                };
            }

            co_with!(|_, mut images| {
                let image = images.get_mut(&handle).unwrap();
                image.sampler_descriptor = ImageSampler::linear();
            });

            // we're done
            std::future::pending::<()>().await;
        }
    )

Some observations:

  • why require the loop? can't we just do nothing in run_unsafe if the future is done?
  • it's easy to want to write code that doesn't work, for example
let load_state = co_with!(|server, _| server.get_load_state(&handle));
let poll = /* Loading => Pending, Loaded => Ready() */;
std::future::poll_fn(|_| poll).await

This doesn't work of course, because the poll is calculated once, and always stays pending.
More accurate would be

std::future::poll_fn(|_| {
    let load_state = co_with!(|server, _| server.get_load_state(&handle));
    /* Loading => Pending, Loaded => Ready() */
}).await

but that doesn't work, hence the loop with yield_now in the final solution.

  • IDE experience is okay-ish, autocomplete works inside co! and co_with, goto definition only works in co! and suggestions like fill match arms don't work at all

Also worth pointing out is that this can live in a third-party crate if we don't want to maintain this ourselves but someone wants to use it, that way we can see how much usage it gets or if there are any more obstactles to async systems.

Comment on lines +310 to +318
// FUTURE: this is the standard hack to poll a future once, but it would
// be more accurate to either provide a "null waker" since we ignore the
// wakeup, or to provide our own waker that allows us to bridge the wake
// to the system's run criteria and only poll once the future is awoken.
// SAFETY:
// - self.pinned.state has no active interior borrows
// - self.pinned.state is valid for get_unchecked_manual within this fn
// given we use the world ref stashed just above (ensured by caller)
if let Some(never) = block_on(poll_once(this.func)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

//! # use bevy_ecs::prelude::*;
//! # use bevy_tasks::prelude::*;
//! # use bevy_async::*;
//! fn make_system() -> impl System {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you return impl System<In = (), Out = ()> it can be added with app.add_system(make_system()).

@jakobhellermann
Copy link
Contributor

I also tried using the nightly generators feature, but unfortunately I don't think that can work.

    || {
        let (asset_server, images) = yield;
        // do stuff
        let (_asset_server, _images) = yield;
        dbg!(&images); // unfortunately still live here
    }

I thought that the system parameters could be resumed into the generator through yield points, but unfortunately old yielded variables are still live after the next yield.

@CAD97
Copy link
Contributor Author

CAD97 commented Nov 16, 2022

feature(generators)

Yeah; using FnPin to illustrate, nightly generators are FnPin(&'exists Args) -> Output, whereas we need for<'all> FnPin(&'all Args) -> Output.

It would be theoretically possible to implement an async state machine desugar in a proc macro, but that's a significant engineering effort that I don't think is worth the effort. Rather, the general use case needs to wait on language-level yield closures that support short-lived arguments. (Streaming is a big use case for them though, so I expect it'll probably happen eventually.)

why require the loop?

Mostly, this is in order to be explicit and avoid making a choice. There's a split on FnPin semantics for what happens after a return; I and the referenced MCP prefer resuming back at the top (this matches the semantics of straightline closures), but it's also a reasonable option to poison the closure (matching futures) making further resumption a panic.

(The former is imho a better option for -> Output, the latter for -> GeneratorState, as the latter differentiates yield and return, but the former doesn't. See the lang team design notes for more context on FnPin.)

Easy to write code that doesn't work

I don't think poll_fn(|_| poll) is that much of a footgun comparatively; anyone who knows what poll_fn does enough to use it would (hopefully) know that this wouldn't recalculate the readiness.

If this async model is adopted more, I suspect we'd add something along the lines of server.poll_ready(&handle)... though needing to write it in a loop still isn't optimal.

loop {
    co_with!(|server, _| server.poll_ready(&handle)).await
}

If bevy were to adopt async even more (though I probably wouldn't recommend it for bevy), we could provide some sort of async channel to notify readiness with a single await.

IDE

Yeah, it's quite nice that the transformation is simple enough that simple IDE functionality works. It's probably worth throwing together the #[coroutine] attribute to see if that improves anything... though being a proc macro it's inherently more difficult to analyze...

3rd party crate

Yeah, although the additional public API surface to bevy_ecs (or similar) in this PR is necessary (as far as I can tell) to be able to implement System for this externally.

@bas-ie
Copy link
Contributor

bas-ie commented Oct 20, 2024

Backlog cleanup: I think if still labelled controversial, and no movement since 2022, it's reasonable to close this one.

@bas-ie bas-ie closed this Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants