Skip to content

Use component_access_set to determine the set of conflicting accesses between two systems. #19495

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
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andriyDev
Copy link
Contributor

@andriyDev andriyDev commented Jun 5, 2025

Objective

Solution

  • Replace component_access with component_access_set when determining conflicting systems during schedule building.
  • All component_access() impls just forward to &component_access_set().combined_access, so we are essentially trading Access::is_compatible for FilteredAccessSet::is_compatible.
  • FilteredAccessSet::get_conflicts internally calls combined_access.is_compatible as the first step, so we can remove that redundant check.

Testing

  • Un-ignored a previously failing test now that it passes!
  • Ran the build_schedule benchmark and got basically no change in the results. Perhaps are benchmarks are just not targetted towards this situation.
$ critcmp main fix-ambiguity -f 'build_schedule'
group                                          fix-ambiguity                          main
-----                                          -------------                          ----
build_schedule/1000_schedule                   1.00       2.9±0.02s        ? ?/sec    1.01       2.9±0.05s        ? ?/sec
build_schedule/1000_schedule_no_constraints    1.02     48.3±1.48ms        ? ?/sec    1.00     47.4±1.78ms        ? ?/sec
build_schedule/100_schedule                    1.00      9.9±0.17ms        ? ?/sec    1.06     10.5±0.32ms        ? ?/sec
build_schedule/100_schedule_no_constraints     1.00   804.7±21.85µs        ? ?/sec    1.03   828.7±19.36µs        ? ?/sec
build_schedule/500_schedule                    1.00    451.7±7.25ms        ? ?/sec    1.04   468.9±11.70ms        ? ?/sec
build_schedule/500_schedule_no_constraints     1.02     12.7±0.46ms        ? ?/sec    1.00     12.5±0.44ms        ? ?/sec

@andriyDev andriyDev added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 5, 2025
Copy link
Contributor

@chescock chescock left a comment

Choose a reason for hiding this comment

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

This seems good to me! I never understood why we used the coarser checking there.

The lack of change in benchmarks seems plausible. FilteredAccessSet::get_conflicts starts by calling combined_access().get_conflicts() and only checks the filters if there is a potential conflict. Any schedule that wasn't reporting conflicts under the old algorithm will pass that first check and not need to do any additional checking.

(And we perform the same FilteredAccessSet::get_conflicts checks in the multi-threaded executor, so I bet we could make a future change to somehow do the checks once and share the answers.)

@andriyDev andriyDev added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 6, 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-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ambiguity reporting does not account for effects of Without query filters, leading to false positives
3 participants