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

Improve ergonomics of Query parallelism #4441

Closed
james7132 opened this issue Apr 8, 2022 · 3 comments
Closed

Improve ergonomics of Query parallelism #4441

james7132 opened this issue Apr 8, 2022 · 3 comments
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible S-Duplicate This issue or PR already exists

Comments

@james7132
Copy link
Member

What problem does this solve or what need does it fill?

Right now calling Query::par_for_each(_mut) requires a TaskPool, a batch size, and a function to run on each entity. This basically requires the system to have a task pool resource and have the user hand tune a batch size.

Ideally both of these arguments should be optional, instead defaulting to common sane defaults for the types and operations involved.

Additionally, the split between par_for_each and for_each forces the user to be aware of the parallelization of the query. Ideally any Query that is heavy enough should be able to transparently split a for_each run into multiple chunks and parallelize across multiple cores. This should enable systems to dynamically scale heavy loads depending on the current demands of the app.

What solution would you like?

  • Make TaskPools either global, or make Query(State) take ownership of a reference to ComputeTaskPool. Remove the TaskPool parameter
  • Have the Query(State) bake in a sane default for a batch size and use builder/fluent APIs to override it, or have the batch size automatically scale to match the current demand by the app.
  • Fold par_for_each and for_each into a single API that transparently chooses to chunk up and parallelize or run entirely single threaded depending on the estimated workload.

What alternative(s) have you considered?

Transparent APIs for iterator composition (i.e. par_iter w/ Iterator-like APIs).

@james7132 james7132 added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Apr 8, 2022
@mockersf
Copy link
Member

mockersf commented Apr 8, 2022

Remove the TaskPool parameter

Just wanted to point out that there are use for task pools outside of queries. It should not be usable only through a query

Fold par_for_each and for_each into a single API

That would make some of the usage of for_each impossible, like simple modification of an outside variable from inside the for_each (without going through the mutex dance)

@superdump superdump moved this to Todo in Rendering Apr 8, 2022
@superdump superdump removed this from Rendering Apr 8, 2022
@james7132
Copy link
Member Author

james7132 commented Apr 8, 2022

Just wanted to point out that there are use for task pools outside of queries. It should not be usable only through a query

This would mostly just be embedding a ComputeTaskPool in QueryState and fetching/cloning the one stored in the World upon creation. Alternatively, having a global static ComputeTaskPool achieves the same thing.

That would make some of the usage of for_each impossible, like simple modification of an outside variable from inside the for_each (without going through the mutex dance)

This is still doable via Query::iter which currently is and should remain as the dominant paradigm for those use cases.

The other main use case of Command construction can be addressed via either making Commands Send + Sync or making a ParallelCommands alternative. I think @TheRawMeatball had some ideas on how to approach this.

@james7132 james7132 added A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels Apr 8, 2022
@james7132 james7132 added the S-Duplicate This issue or PR already exists label May 26, 2022
@james7132
Copy link
Member Author

Closing this out as this is redundant with the already pre-existing #3183 and #3184.

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 S-Duplicate This issue or PR already exists
Projects
None yet
Development

No branches or pull requests

2 participants