Skip to content

Optimize validate_param #15505

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

Open
MiniaczQ opened this issue Sep 28, 2024 · 3 comments
Open

Optimize validate_param #15505

MiniaczQ opened this issue Sep 28, 2024 · 3 comments
Labels
A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times D-Unsafe Touches with unsafe code in some way S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Controversial There is active debate or serious implications around merging this PR

Comments

@MiniaczQ
Copy link
Contributor

MiniaczQ commented Sep 28, 2024

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

From discord discussion: https://discord.com/channels/691052431525675048/749335865876021248/1289681891611508809

As @cart pointed out, validate_param doubles lookup of get_param, which can have significant performance impact.

What solution would you like?

There are few potential ways of fixing that:

  1. Merge get_param with validate_param into try_get_param.
    1.1. In naive approach, we always create a system task (multithreaded executor), even if parameters can't be retrieved.
    1.2. We can try to run try_get_param before spawning a task, but then we need to somehow pass the parameters into the system. (type erasure issues)
  2. Keep get_param and validate_param, add try_get_param with default implementation that merges the previous 2.
    Implement manually for performance gains.
@MiniaczQ MiniaczQ added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR and removed S-Needs-Triage This issue needs to be labelled labels Sep 28, 2024
@MiniaczQ MiniaczQ added this to the 0.15 milestone Sep 28, 2024
@MiniaczQ MiniaczQ added S-Blocked This cannot move forward until something else changes D-Unsafe Touches with unsafe code in some way C-Performance A change motivated by improving speed, memory usage or compile times S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed C-Feature A new feature, making something new possible S-Blocked This cannot move forward until something else changes labels Sep 28, 2024
@alice-i-cecile alice-i-cecile assigned cart and unassigned cart Oct 1, 2024
@alice-i-cecile
Copy link
Member

I prefer solution 2 here.

@SpecificProtagonist
Copy link
Contributor

SpecificProtagonist commented Oct 1, 2024

To summarize, the difficulty lies in the multithreaded case, as validation and fetching happen on different tasks:
1.1. Speeds up the case that the system runs at the expense of the case it doesn't.
1.2. Type erasure isn't the issue (the system knows their type) – the problem is that they have the lifetime of the state & world used to fetch them, so we can't store them and hand them off to another task
2. Does not help with the multithreaded executor. It also has the most code duplication.

While variant 1.2. is a bit tricky, it's best fit.

@MiniaczQ
Copy link
Contributor Author

All the presented solutions suffer from the same issue:
combinator systems (which consist of many subsystems, exclusive ones included), can invalidate their own params
a.pipe(b).pipe(a), b can remove params required for second a.

This means, we can never enforce that the entire System (defined by the trait) can run in it's entirety.
If we drop this requirement and allow combinator systems to run "until they can't",
then multithreaded executor only has to check whether the first subsystem can run.

This new solution is implemented in #15606

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-Performance A change motivated by improving speed, memory usage or compile times D-Unsafe Touches with unsafe code in some way S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
4 participants