[Merged by Bors] - Converted exclusive systems to parallel systems wherever possible #2774

SarthakSingh31 · 2021-09-05T06:24:08Z

Closes #2767.

Converted:

play_queued_audio_system
change_window

alice-i-cecile

Awesome, thanks for doing this! Left one nit, but it looks great.

We should verify that we're not introducing one-frame delays with new execution order ambiguities. I'm a bit loathe to ask you to do that in this PR; feel free to investigate if you'd like but if it's beyond you we can do a proper cleanup pass for that in a seperate PR.

crates/bevy_winit/src/lib.rs

SarthakSingh31 · 2021-09-05T18:16:07Z

It is probably introducing ambiguity and some one-frame delays. From what I understand by default all the exclusive systems run at the start of the stage and this change makes it run in between. I would assume the same applies to gilrs_event_system.

I don't know how to make these systems run first without making them exclusive again or putting labels everywhere. Idk if putting labels everywhere is an acceptable solution, I don't think it is very maintainable.

For Reference:

Ambiguity before this change:

-- "bevy_winit::change_window" and "bevy_audio::audio_output::play_queued_audio_system<bevy_audio::audio_source::AudioSource>"

Ambiguity after this change:

-- "bevy_render::camera::camera::camera_system<bevy_render::camera::projection::OrthographicProjection>" and "bevy_winit::change_window"
-- "bevy_render::camera::camera::camera_system<bevy_render::camera::projection::PerspectiveProjection>" and "bevy_winit::change_window"
-- "bevy_ui::widget::text::text_system" and "bevy_winit::change_window"
-- "bevy_ui::flex::flex_node_system" and "bevy_winit::change_window"
-- "bevy_winit::change_window" and "bevy_text::text2d::text2d_system"

alice-i-cecile · 2021-09-05T18:45:57Z

Idk if putting labels everywhere is an acceptable solution, I don't think it is very maintainable.

Yeah; there's a larger conversation ongoing about this: see #2747.

I suspect that the change_window system's ambiguities are going to genuinely cause issues. Can we move this into the stage before?

SarthakSingh31 · 2021-09-05T21:48:36Z

Can we move this into the stage before?

I think that will exacerbate the issue of one-frame delays because the majority of commands put into the Window::command_queue will probably come from CoreStage::Update. It might be best to keep it as an exclusive system for now. It is hard to say which is better without more testing.

SarthakSingh31 · 2021-09-06T18:14:39Z

@alice-i-cecile I did some testing with timing the duration it takes for changes triggered in scale_factor_override.rs example to show up in the flex_node_system (the only consumer of WindowScaleFactorChanged events).

postupdate parallel: 0.549728 ms - 10.027739 ms
postupdate exlcusive: 0.390465 ms - 0.620356 ms
update parallel: 0.390914 ms - 7.55568875 ms 
update exclusive: 7.643498 ms - 11.235118 ms

I think the parallel ones change the order they are executed in across executions of the binary. They produce extremely varying results.

alice-i-cecile · 2021-09-06T18:27:43Z

because the majority of commands put into the Window::command_queue will probably come from CoreStage::Update

Hmm, I agree. We could make another stage :/ I think the best solution here is probably to add a public label to change_window and then have all the other conflicting systems run before it.

I think the parallel ones change the order they are executed in across executions of the binary. They produce extremely varying results.

Yep, that's standard ambiguous ordering behavior :(

SarthakSingh31 · 2021-09-06T21:30:13Z

@alice-i-cecile Added the labels in. Ran a benchmark again and now its around 0.302998 ms - 0.411899 ms pretty reliably. There are no more ambiguities reported related to this pr now.

alice-i-cecile

Fantastic; looks good to me now! Thanks for tackling this.

crates/bevy_audio/src/audio_output.rs

crates/bevy_window/src/lib.rs

alice-i-cecile · 2022-02-28T15:10:37Z

@SarthakSingh31 can you rebase this on main? :) I'd like to get this merged but there are now merge conflicts

SarthakSingh31 · 2022-03-01T01:55:03Z

@alice-i-cecile rebased. It looks like gilrs_event_system was converted in a different pr so removed anything related to it from this pr.

mockersf · 2022-03-01T19:08:21Z

The system add_clusters in bevy_pbr is an exclusive system that uses Res<Windows> and that would be executed before the change_window system after this change. Is that an issue? @superdump ?

alice-i-cecile · 2022-03-01T19:25:09Z

The system add_clusters in bevy_pbr is an exclusive system that uses Res and that would be executed before the change_window system after this change. Is that an issue?

My intution is that this should run after, both to preserve existing behavior and for correctness. @SarthakSingh31 can you make that change? We can revert it if our rendering experts tell us otherwise.

superdump · 2022-03-01T19:34:59Z

The system add_clusters in bevy_pbr is an exclusive system that uses Res<Windows> and that would be executed before the change_window system after this change. Is that an issue? @superdump ?

Hmm. I think the current setup has add_clusters being run first as an exclusive system to ensure the presence of Clusters components on appropriate views, and then update_clusters is a normal system that overwrites the cluster configuration based on the window size. I think that suggests it only has to be correct when update_clusters is run. To complicate things further, I know that update_clusters is going to move to being called from assign_lights_to_clusters in an upcoming PR.

To be clear, add_clusters needs to be run such that the Clusters components are actually added (ie commands are applied to the world) before any of the subsequent clustered forward rendering PostUpdate systems are run. And I think the window sizes only have to be correct at the point of execution of update_clusters currently but it would have to be tested. If possible just preserve current behaviour.

superdump · 2022-03-01T19:47:45Z

If add_clusters required windows to be correct and it is an exclusive system, then it would then require that change_windows is either exclusive and run before it, or it is run in an earlier stage, right?

alice-i-cecile · 2022-03-01T20:18:41Z

If add_clusters required windows to be correct and it is an exclusive system, then it would then require that change_windows is either exclusive and run before it, or it is run in an earlier stage, right?

Huh, change_window is also an exclusive system. In that case, yes.

However, it looks like change_window also has no reason to be exclusive:

bevy/crates/bevy_winit/src/lib.rs

Line 48 in caf6611

fn change_window(world: &mut World) {

.

mockersf · 2022-03-01T20:28:47Z

yup, this pr is changing change_window from exclusive to non exclusive

it seems add_clusters is set as exclusive only for ordering purpose though

superdump · 2022-03-01T22:40:28Z

yup, this pr is changing change_window from exclusive to non exclusive

it seems add_clusters is set as exclusive only for ordering purpose though

It isn't only for ordering. It inserts Clusters components on views which don't have them. Subsequent systems in PostUpdate expect Clusters to be present. I suppose if you accepted a 1-frame delay in the Clusters component coming into existence then it would be fine.

That said, I don't think add_clusters was specified to necessarily run after change_windows among the exclusive PostUpdate systems, so it could have run after. And, also as said, I think as long as update_clusters runs after change_windows, it should be fine, but we need to test it.

SarthakSingh31 · 2022-03-03T06:23:22Z

@mockersf fixed the ordering and resolved the new merge conflicts. I feel like update_clusters should be reported as ambiguous before this change but it isn't.

superdump

I left a couple of comments.

Separately, continuing the add_clusters discussion, I think before this PR, add_clusters would run after change_windows with DefaultsPlugins plugin ordering because winit would be initialised before pbr so the change_windows exclusive system would be registered before the add_clusters exclusive system and so change_windows would also be executed before add_clusters.

With this PR change_windows is being moved to after add_clusters because add_clusters is still exclusive and change_windows is not.

As mentioned, I think the important things from a practical perspective are that add_clusters remains exclusive so that clusters components get added to appropriate views before update_clusters is later executed, and change_windows must run before update_clusters. The labelling of change_windows and the ordering condition that update_clusters be run after change_windows satisfies this.

superdump · 2022-03-03T07:28:32Z

crates/bevy_pbr/src/lib.rs

@@ -80,14 +82,16 @@ impl Plugin for PbrPlugin {
                // add as an exclusive system
                add_clusters
                    .exclusive_system()
-                    .label(SimulationLightSystems::AddClusters),
+                    .label(SimulationLightSystems::AddClusters)
+                    .after(ModifiesWindows),


I think now that change_windows is not exclusive, this will cause a warning to be logged that this system tried to be scheduled after ModifiesWindows and that system doesn’t exist in the exclusive part of PostUpdate.

superdump · 2022-03-03T07:39:05Z

crates/bevy_window/src/lib.rs

@@ -73,3 +73,6 @@ impl Plugin for WindowPlugin {
        }
    }
 }
+
+#[derive(Debug, Hash, PartialEq, Eq, Clone, SystemLabel)]
+pub struct ModifiesWindows;


Can systems have multiple labels, if not then the approach of tagging a system with things it does doesn’t really hold because a system could modify multiple things as well as the windows.

Yes, multiple levels are permitted.

Ok. Then this is not a problem.

alice-i-cecile · 2022-04-25T13:01:26Z

@SarthakSingh31 can you rebase this? I'd like to merge this in.

alice-i-cecile · 2022-04-25T14:09:10Z

bors r+

) Closes #2767. Converted: - `play_queued_audio_system` - `change_window`

IceSentry · 2022-04-25T14:49:17Z

I don't know if it's too late now, but it would be nice if this PR used the actual system as a label instead of a new SystemLabel now that we have this feature.

edit: it's too late now

bors · 2022-04-25T14:50:36Z

Pull request successfully merged into main.

Build succeeded:

…vyengine#2774) Closes bevyengine#2767. Converted: - `play_queued_audio_system` - `change_window`

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Sep 5, 2021

SarthakSingh31 changed the title ~~Convert exclusive systems to parallel systems wherever possible~~ Converted exclusive systems to parallel systems wherever possible Sep 5, 2021

NiklasEi added C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times and removed S-Needs-Triage This issue needs to be labelled labels Sep 5, 2021

alice-i-cecile approved these changes Sep 5, 2021

View reviewed changes

crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved

alice-i-cecile approved these changes Sep 6, 2021

View reviewed changes

afonsolage approved these changes Sep 6, 2021

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 7, 2021

mockersf reviewed Sep 11, 2021

View reviewed changes

crates/bevy_audio/src/audio_output.rs Outdated Show resolved Hide resolved

SarthakSingh31 requested a review from mockersf September 13, 2021 01:57

SarthakSingh31 force-pushed the min_ex_system branch 2 times, most recently from e135d8d to 9a0c1b2 Compare September 15, 2021 01:22

SarthakSingh31 requested a review from alice-i-cecile September 15, 2021 01:46

alice-i-cecile approved these changes Sep 15, 2021

View reviewed changes

mockersf approved these changes Sep 15, 2021

View reviewed changes

IceSentry reviewed Sep 16, 2021

View reviewed changes

crates/bevy_window/src/lib.rs Show resolved Hide resolved

IceSentry approved these changes Sep 16, 2021

View reviewed changes

SarthakSingh31 force-pushed the min_ex_system branch 4 times, most recently from 7c54787 to 562dc29 Compare March 1, 2022 01:53

SarthakSingh31 force-pushed the min_ex_system branch 2 times, most recently from 662d429 to 4f4b478 Compare March 3, 2022 05:59

superdump requested changes Mar 3, 2022

View reviewed changes

superdump reviewed Mar 3, 2022

View reviewed changes

alice-i-cecile mentioned this pull request Mar 31, 2022

Tracking issue: eliminate or explicitly silence all Bevy-internal execution order ambiguities #4377

Closed

11 tasks

SarthakSingh31 added 3 commits April 25, 2022 06:06

Converted play_queued_audio_system to be parallel

f834cdc

Converted change_window to be parallel

ad3c330

Added ModifiesWindows to resolve ambiguities caused by change_window

f04d91c

SarthakSingh31 force-pushed the min_ex_system branch from 4f4b478 to f04d91c Compare April 25, 2022 13:31

SarthakSingh31 requested review from superdump and alice-i-cecile April 25, 2022 14:01

alice-i-cecile approved these changes Apr 25, 2022

View reviewed changes

bors bot pushed a commit that referenced this pull request Apr 25, 2022

Converted exclusive systems to parallel systems wherever possible (#2774

5155034

) Closes #2767. Converted: - `play_queued_audio_system` - `change_window`

bors bot changed the title ~~Converted exclusive systems to parallel systems wherever possible~~ [Merged by Bors] - Converted exclusive systems to parallel systems wherever possible Apr 25, 2022

bors bot closed this Apr 25, 2022

SarthakSingh31 deleted the min_ex_system branch April 25, 2022 15:05

exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022

Converted exclusive systems to parallel systems wherever possible (be…

2e0c4ae

…vyengine#2774) Closes bevyengine#2767. Converted: - `play_queued_audio_system` - `change_window`

ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023

Converted exclusive systems to parallel systems wherever possible (be…

6e36c3d

…vyengine#2774) Closes bevyengine#2767. Converted: - `play_queued_audio_system` - `change_window`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Converted exclusive systems to parallel systems wherever possible #2774

[Merged by Bors] - Converted exclusive systems to parallel systems wherever possible #2774

SarthakSingh31 commented Sep 5, 2021 •

edited

Loading

alice-i-cecile left a comment

SarthakSingh31 commented Sep 5, 2021 •

edited

Loading

alice-i-cecile commented Sep 5, 2021

SarthakSingh31 commented Sep 5, 2021

SarthakSingh31 commented Sep 6, 2021 •

edited

Loading

alice-i-cecile commented Sep 6, 2021

SarthakSingh31 commented Sep 6, 2021 •

edited

Loading

alice-i-cecile left a comment

alice-i-cecile commented Feb 28, 2022

SarthakSingh31 commented Mar 1, 2022

mockersf commented Mar 1, 2022 •

edited

Loading

alice-i-cecile commented Mar 1, 2022 •

edited

Loading

superdump commented Mar 1, 2022 •

edited

Loading

superdump commented Mar 1, 2022

alice-i-cecile commented Mar 1, 2022

mockersf commented Mar 1, 2022

superdump commented Mar 1, 2022

SarthakSingh31 commented Mar 3, 2022

superdump left a comment

superdump Mar 3, 2022

SarthakSingh31 Apr 25, 2022

superdump Mar 3, 2022 •

edited

Loading

alice-i-cecile Mar 3, 2022

superdump Mar 6, 2022

alice-i-cecile commented Apr 25, 2022

alice-i-cecile commented Apr 25, 2022

IceSentry commented Apr 25, 2022 •

edited

Loading

bors bot commented Apr 25, 2022

[Merged by Bors] - Converted exclusive systems to parallel systems wherever possible #2774

[Merged by Bors] - Converted exclusive systems to parallel systems wherever possible #2774

Conversation

SarthakSingh31 commented Sep 5, 2021 • edited Loading

alice-i-cecile left a comment

Choose a reason for hiding this comment

SarthakSingh31 commented Sep 5, 2021 • edited Loading

alice-i-cecile commented Sep 5, 2021

SarthakSingh31 commented Sep 5, 2021

SarthakSingh31 commented Sep 6, 2021 • edited Loading

alice-i-cecile commented Sep 6, 2021

SarthakSingh31 commented Sep 6, 2021 • edited Loading

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Feb 28, 2022

SarthakSingh31 commented Mar 1, 2022

mockersf commented Mar 1, 2022 • edited Loading

alice-i-cecile commented Mar 1, 2022 • edited Loading

superdump commented Mar 1, 2022 • edited Loading

superdump commented Mar 1, 2022

alice-i-cecile commented Mar 1, 2022

mockersf commented Mar 1, 2022

superdump commented Mar 1, 2022

SarthakSingh31 commented Mar 3, 2022

superdump left a comment

Choose a reason for hiding this comment

superdump Mar 3, 2022

Choose a reason for hiding this comment

SarthakSingh31 Apr 25, 2022

Choose a reason for hiding this comment

superdump Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

alice-i-cecile Mar 3, 2022

Choose a reason for hiding this comment

superdump Mar 6, 2022

Choose a reason for hiding this comment

alice-i-cecile commented Apr 25, 2022

alice-i-cecile commented Apr 25, 2022

IceSentry commented Apr 25, 2022 • edited Loading

bors bot commented Apr 25, 2022

SarthakSingh31 commented Sep 5, 2021 •

edited

Loading

SarthakSingh31 commented Sep 5, 2021 •

edited

Loading

SarthakSingh31 commented Sep 6, 2021 •

edited

Loading

SarthakSingh31 commented Sep 6, 2021 •

edited

Loading

mockersf commented Mar 1, 2022 •

edited

Loading

alice-i-cecile commented Mar 1, 2022 •

edited

Loading

superdump commented Mar 1, 2022 •

edited

Loading

superdump Mar 3, 2022 •

edited

Loading

IceSentry commented Apr 25, 2022 •

edited

Loading