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

Use single threaded executor in benchmarks and tests #9908

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

A-Walrus
Copy link
Contributor

Objective

Fixes: #9838

Solution

Add single_threaded constructor for Schedule and use it in all benchmarks and tests.

@hymm
Copy link
Contributor

hymm commented Sep 23, 2023

You shouldn't make this change for any of those scheduling benchmarks. Those are specifically measuring multithreaded execution.

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

Can you also do a pass over the doc examples? Those get run during testing too. A lot of them are using Schedule::default.

@@ -791,7 +791,7 @@ mod tests {
}
}

let mut schedule = Schedule::default();
let mut schedule = Schedule::default(); // Uses MultiThreaded executor
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this should use the multithreaded schedule? This seems to just be testing the parallel iterator, which is unrelated to multithreaded scheduling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I tried it with the singlethreaded schedule it didn't work... might have accidentaly uncovered a bug :P

Copy link
Contributor

Choose a reason for hiding this comment

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

I changed it to single_threaded on my machine and wasn't able to get it to fail. Could you double check and post the error if it does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, changed it to single_threaded and it seems to work fine :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

---- query::tests::par_iter_mut_change_detection stdout ----
thread 'query::tests::par_iter_mut_change_detection' panicked at 'A ComputeTaskPool has not been initialized yet. Please call ComputeTaskPool::init beforehand.', crates/bevy_tasks/src/usages.rs:28:33
Encountered a panic in system `bevy_ecs::query::tests::par_iter_mut_change_detection::propagate_system`!


failures:
    query::tests::par_iter_mut_change_detection

Got a failure for this test on the latest commit. Might be flaky...

Left the visible ones as default in order to not make things confusing.
The invisible ones (marked with `/// #`) I changed to single_threaded.
@hymm
Copy link
Contributor

hymm commented Sep 23, 2023

I think we should make a multithreaded method that just wraps default to make it explicit we're choosing the multithreaded executor instead of leaving a comment.

@JMS55
Copy link
Contributor

JMS55 commented Sep 23, 2023

Same for rendering benchmarks, we want those to be multithreaded.

@A-Walrus
Copy link
Contributor Author

A-Walrus commented Sep 24, 2023

Same for rendering benchmarks, we want those to be multithreaded.

@JMS55 Which rendering benchmarks are you referring to? I couldn't find any in the benches dir.

@JMS55
Copy link
Contributor

JMS55 commented Sep 24, 2023

Ah nvm then. All the rendering "benchmarks" are actually examples, not part of benches.

@alice-i-cecile alice-i-cecile requested a review from hymm September 29, 2023 08:15
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This looks like the right setup now to me.

@alice-i-cecile
Copy link
Member

@A-Walrus that CI failure looks like it might be real: can you reproduce it locally?

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Testing A change that impacts how we test Bevy or how users test their apps labels Sep 29, 2023
@A-Walrus
Copy link
Contributor Author

@alice-i-cecile It's passing locally, but it has failed in CI multiple times. Weird

@hymm
Copy link
Contributor

hymm commented Sep 30, 2023

So we should either do like the error is saying and call ComputeTaskPool::init() before running the test or change par_for_each_unchecked_manual to call ComputeTaskPool::init(). I suspect the former is more correct, since that gives the user a chance to init the task pool how they like. We do call init when we're using the multithreaded executor. @james7132 do you have any opinion on this?

@BenjaminBrienen BenjaminBrienen added D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 23, 2025
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-Testing A change that impacts how we test Bevy or how users test their apps D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use single-threaded executor in benchmarks by default
5 participants