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

Make scheduling benchmark more realistic #5410

Closed
wants to merge 8 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Jul 21, 2022

Objective

  • Current schedule bench is quite unrealistic.
  • Redesign it based on the architecture of a typical bevy app.

Solution

Model the benchmarks around plugins, forming groups of systems.

  • Systems within a group have orderings specified based on their TypeId.
  • Inter-system dependencies are based on derived labels.
  • Plugins have their own labels, which is passed down to all systems they contain. This allows plugin-wide dependencies sometimes.

@JoJoJet JoJoJet marked this pull request as ready for review July 21, 2022 07:11
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events A-Diagnostics Logging, crash handling, error reporting and performance analysis labels Jul 21, 2022
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.

Love the constants, love the strategy for defining the graphs, love the increased realism. Eventually I'd be interested in a follow-up PR that adds a method to compute these key statistics on Schedules so we can ask users for usage data.

I think we can make this nicer with code generation macros, and strongly feel that this needs to be explicitly seeded.

Should be great!

benches/benches/bevy_ecs/scheduling/schedule.rs Outdated Show resolved Hide resolved
benches/benches/bevy_ecs/scheduling/schedule.rs Outdated Show resolved Hide resolved
benches/benches/bevy_ecs/scheduling/schedule.rs Outdated Show resolved Hide resolved
}

fn my_system<const P: usize, const I: usize>() {}

Copy link
Member

Choose a reason for hiding this comment

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

I would like the number of plugins and systems to be pulled out into constants here.

Copy link
Member Author

@JoJoJet JoJoJet Jul 21, 2022

Choose a reason for hiding this comment

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

The problem with this is that, afaik, every single const generic type needs to be explicitly named somewhere. You can't loop over const generics. I can make it prettier with macros, but I don't think it's possible to extract everything into constants.

I can think of a few potential solutions, but they will require re-architecting.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that's surprising 🤔 I'm not familiar enough with the details of macros to know if what I'm asking for is possible. We should see if we can get more expert opinions.

Copy link
Member

@alice-i-cecile alice-i-cecile Jul 21, 2022

Choose a reason for hiding this comment

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

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 has my approval now :) I'd still like to be able to extract out the number of systems / plugins into constants, but I won't block on it in this PR if you can't get it working.

@JoJoJet
Copy link
Member Author

JoJoJet commented Jul 21, 2022

I'd still like to be able to extract out the number of systems / plugins into constants, but I won't block on it in this PR if you can't get it working

So here's what I've gathered so far.

  • seq_macro only works for literal consts, not const generics.
  • Refactoring the code to not use const generics adds a ton of noise to the benchmarks (maybe because the compiler can't unroll the system-adding loops?). incorrect

How would you feel about moving the system-adding code out of benchmark, and only timing the first call to SystemStage::run? Originally I wanted to include add_system() calls in the benchmarks since that is a real part of bevy apps, but it might not be worth it.

@alice-i-cecile
Copy link
Member

Sounds good. We can clean this up more later.

@JoJoJet
Copy link
Member Author

JoJoJet commented Sep 11, 2023

Closing this since it's very out of date. If someone wants to update or replicate this, feel free.

@JoJoJet JoJoJet closed this Sep 11, 2023
@JoJoJet JoJoJet deleted the better-bench branch September 11, 2023 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis A-ECS Entities, components, systems, and events
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants