Skip to content
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

Async system support #1393

Closed

Conversation

TheRawMeatball
Copy link
Member

(Pinging @DJMcNab because this idea was from him)

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I think possibly this one change would be sufficient to make it safe

Hopefully

crates/bevy_ecs/src/system/async_system.rs Outdated Show resolved Hide resolved
@cart
Copy link
Member

cart commented Feb 3, 2021

Ok this is really cool and in general looks good to me. I like this impl because it allows you to encapsulate "before", "during", and "after" steps in the same system. But for thoroughness its worth considering additional apis before merging. The Accessor enables that encapsulation, but I think it could probably be fully abstracted out with system chaining:

app.add_system(
    before.system().chain(
        during.system().chain(
            after.system()
        )
    )
)

fn before(asset_server: Res<AssetServer>) -> AssetServer {
    // asset server is cloneable and thread safe
    asset_server.clone()
}

async during(In(asset_server): In<AssetServer>) -> Option<AssetPathId> {
    if let Ok(asset_path_id) = asset_server.load_async().await {
        Some(asset_path_id)
    } else {
        warn!("failed to load asset");
        None
    }
}

fn after(In(loaded_path): In<Option<AssetPathId>>, mut foo: ResMut<Foo>) {
    foo.do_thing(loaded_path);
}

alternative less nest-ey construction:

app.add_system(AsyncSystem {
    before: before.system(),
    during: during.system(),
    after: after.system(),
})

@TheRawMeatball
Copy link
Member Author

Hmm, this looks quite nice but I'm not super sold. It feels like its been turned inside-out. As an alternative to this, what would you think about such an API? (this example is using apis from #1144 to exemplify future expansion)

async fn async_system(pre: Accessor<Res<AssetServer>>, post: Accessor<ResMut<Foo>>) {}

app.add_async_system(async_system.synced_at(|(sync_1, sync_2)| {
    (
        (stage::UPDATE, sync_1.before("bar_system")),
        (stage::POST_UPDATE, sync_2.after("baz_system")),
    )
}));

I feel like this could be more extensible. As one final note, if async systems are able to wait for a frame like they can with Accessor, they can enter infinite loops without a problem and trivially hold extra state. Async-specific state in a chain-based system would likely be more contrived.

@cart
Copy link
Member

cart commented Feb 3, 2021

It feels like its been turned inside-out.

Haha from my perspective its the Accessor api that is inside out. We must run the "sync" logic at the point where we've inserted the system in the schedule. We already have a construct for doing that (normal systems). Its the async logic that needs to handled differently. But maybe I just need to think about this more. I'll spend some more time on this soon.

synced_at

The "synced_at" api seems like it is way overcomplicating things. It adds new builder apis, new ways to run sync logic, and seems harder to wrap your head around in general. I think I'd prefer this pr's current approach in almost every case. Cross stage/dependency logic could be done the same way it is now (modifying state in other systems).

Async-specific state in a chain-based system would likely be more contrived.

I think it would be pretty straightforward. I think we could even remove the channel entirely in favor of polling the returned future manually. (warning: lazy pseudocode ahead) Something like:

struct ChainSyncToAsyncSystem<Input, Output> {
   sync_system: Box<dyn System<In = Input, Out = T>>,
   async_system: Box<dyn AsyncSystem<In = T, Out = Output>>,
   future: Option<BoxedFuture<Output = Output>>,
} 

impl<Input, Output> System<Input, Option<Output>> for ChainSyncToAsyncSystem<Input, Output> {
    /* other impls here */
    fn run_unsafe(&mut self, world: &mut World, resources: &mut Resources) {
        if let Some(future) = self.future {
            match future.poll() {
                Poll::Ready(value) => Some(value),
                Poll::Pending => None 
            }
        } else {
            let out = self.sync_system.run_unsafe(world, resources);
            let task_pool = resources.get_task_pool();
            self.future = Some(task_pool.spawn(async_system))
            None
        }
    }
}

@bjorn3
Copy link
Contributor

bjorn3 commented Feb 3, 2021

The chain based version wouldn't work when the world state is accessed inside a loop thatbalso contains .await.

@cart
Copy link
Member

cart commented Feb 3, 2021

Ok I think I see your point. I see the value in having a "higher level" async system as the "orchestrator" of sync execution to manage the outputs more naturally. The "two perspectives" being discussed here each optimize for different use cases. It might be worth going through the thought experiment of coming up with common scenarios and trying to implement each scenario with both approaches.

Ex this is what this pr's timer example would look like:

app.add_system(wait_duration.chain(
    timer.system().on_finish(
        greet.system()
    )
)

fn wait_duration(wait_duration: Res<WaitDuration>) -> Duration {
    wait_duration.duration()
}

async fn timer(In(wait_duration): In<Duration>) {
    futures_timer::Delay::new(Duration::from_secs_f32(wait_duration)).await;
}

fn greet(greet_msg: Res<GreetMessage>, mut next_delay: Res<NextGreetDelay>, time: Res<Time>, execution_time: Res<ExecutionTime>, wait_duration: ResMut<WaitDuration>) {
    next_delay.0 += 1.;
    *wait_duration = if next_delay.0 > time.delta_seconds() {
        sync_operation(&greet_msg.0, execution_time.0);
        next_delay.0 - time.delta_seconds();
    } else {
        // We had a lag spike, and the frame time exceeded our waiting.
        let mut timer = time.delta_seconds();
        while timer > next_delay.0 {
            timer -= next_delay.0;
            // Do multiple operations to catch up.
            // (This is an example so we don't actually have anything to do)
            sync_operation(&greet_msg.0, execution_time.0)
        }
        timer
    };
}

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Feb 3, 2021

Other potential uses (for mocking up and release notes / docs):

  • Systems operating on a fixed time step with continuous computation, rather than the current just-in-time model
  • Networking
  • Decoupled physics and rendering (see Frame rate limiting #1343)
  • Expensive, non-urgent computations that take more than a frame (e.g. pathfinding)
  • Asset loading

Further stretch use cases:

  • Asynchronous handling of atomic tasks (like cleanup, or possibly Commands) as work is generated for them

@TheRawMeatball
Copy link
Member Author

The "synced_at" API seems like it is way overcomplicating things.

Looking back, its definitely overcomplicated. But, I do think being able to coordinate with multiple sync points, located in arbitrary positions in the schedule is very valuable, so I made a pure extension to the API: async systems with just one accessor can still be added using the current API, but in this branch I added a new function:

fn systems(self) -> Vec<BoxedSystem>

This way, we lose no simplicity and the user can easily add these sync points wherever they want.

@TheRawMeatball
Copy link
Member Author

On an unrelated note, what about adding some bevy-specific future types? For example, what about a future that returns from await when a resource changes, or an event fires to cancel a compute task that is no longer necessary?

@Epitrochoid
Copy link

I had a question in the Discord and was suggested that it could make for a use case for async systems.

For loading a map I have an asset-depends-on-another-asset sequencing problem. The map descriptor (json/text file) contains paths to the textures for each tilemap. Async startup systems (and access to the individual asset Futures) would let me sequence loading the descriptor, loading the textures in that descriptor, then building the object representation of the map. It would be especially convenient to be able to get a Future from the asset server that continues when all loading assets are finished.

@TheRawMeatball
Copy link
Member Author

Hmm, currently I don't think this has support for startup / oneshot systems, but this is definitely a use case we'll want, so I'll think about it.

@TheRawMeatball
Copy link
Member Author

Hmm, I just had an idea thats worth considering: since async systems can trivially implement looping behavior internally, what if we made them one-shot by default? In such a world, an async system would be a group of "sync systems" which are regular systems, and an async system which is an async function that can await a give sync system to wait for and more generally manage them.
If they were one-shot by default, the async system could be started by sending a special type of event with a label, and them a if a sync system receives this event, it starts up the async system.

@alice-i-cecile alice-i-cecile added core A-ECS Entities, components, systems, and events labels Feb 17, 2021
@TheRawMeatball TheRawMeatball removed the S-Blocked This cannot move forward until something else changes label Feb 24, 2021
@TheRawMeatball TheRawMeatball marked this pull request as ready for review March 6, 2021 10:25
@anarelion
Copy link
Contributor

The only problem that I see here is that async is meant to do blocking IO. And blocking is something that you are supposed to never do in a tightly controlled loop that needs to run ever 1/60th second or faster.

For the case of loading a map with all those dependencies, what I have done and you could do is to load them as a scene.

@TheRawMeatball
Copy link
Member Author

Async isn't necessarily about IO - it could also be used for long-running CPU-intensive workloads that need to span multiple frames.
Also, the main loop will not block on their completion, meaning as long as this yields regularly, it can also handle IO, though we'd probably have to make an async AssetServer API to simplify this.

@anarelion
Copy link
Contributor

I think there are 2 different concepts here.

  • Async as in the technology: This concept was invented to avoid expensive context switches and help with IO heavy processes. If you do heavy CPU computations in a async event loop, you are stopping other tasks from running, which defeats the whole concept of it and kills some other async processes expectations. You might end up starving the Asset Loading because you are calculating a complex path.
  • Async as in the concept of running things outside of the game loop: Yes, this is it. Later at some point, you could send an event to the game loop (system) and process it in a system. This doesn't really need to use async the technology.

@TheRawMeatball
Copy link
Member Author

Async is a general purpose tool to have computations that can yield to a larger cycle, either in regular intervals or to wait for an IO operation. Here, async the techology gives us an easy way to have computations that can yield, or await for the main game loop, hence their use here.

@alice-i-cecile
Copy link
Member

I think there are 2 different concepts here.

  1. Async as in the technology: This concept was invented to avoid expensive context switches and help with IO heavy processes. If you do heavy CPU computations in a async event loop, you are stopping other tasks from running, which defeats the whole concept of it and kills some other async processes expectations. You might end up starving the Asset Loading because you are calculating a complex path.
  2. Async as in the concept of running things outside of the game loop: Yes, this is it. Later at some point, you could send an event to the game loop (system) and process it in a system. This doesn't really need to use async the technology.

Useful message thread in Discord, following up on this.

@TheRawMeatball
Copy link
Member Author

I'm closing this PR in favor of the smaller and simpler Accessor PR in #1900.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants