[Merged by Bors] - Make `System` responsible for updating its own archetypes #4115

tornewuff · 2022-03-05T17:26:21Z

Objective

Make it possible to use Systems outside of the scheduler/executor without having to define logic to track new archetypes and call System::add_archetype() for each.

Solution

Replace System::add_archetype(&Archetype) with System::update_archetypes(&World), making systems responsible for tracking their own most recent archetype generation the way that SystemState already does.

This has minimal (or simplifying) effect on most of the code with the exception of FunctionSystem, which must now track the latest ArchetypeGeneration it saw instead of relying on the executor to do it.

tornewuff · 2022-03-05T17:27:00Z

This was suggested in the discussion on https://discord.com/channels/691052431525675048/947636849139150868 with @alice-i-cecile and others - it turned out to be pretty simple, so sending this to see what folks think.

maniwani · 2022-03-05T17:32:18Z

Instead of replacing the new_archetype method, can we just add your update_archetypes method?

Some users (and mocking up tests) might still want to register only specific archetypes.

crates/bevy_ecs/src/schedule/executor_parallel.rs

crates/bevy_ecs/src/system/function_system.rs

hymm

Overall as long as this doesn't change perf too much, I think it makes sense as a low level api for people to run systems outside of the executor.

hymm · 2022-03-05T18:30:13Z

crates/bevy_ecs/src/system/mod.rs

-        for archetype in world.archetypes.iter() {
-            system.new_archetype(archetype);
-        }
+        system.update_archetypes(&world);


This test doesn't really check if the archetypes are updated correctly. Since this new function is part of the public api that we expect to be used, should we be adding more tests to check correctness?

crates/bevy_ecs/src/schedule/executor.rs

crates/bevy_ecs/src/system/function_system.rs

alice-i-cecile

Overall I like this change quite a bit:

Positive:

less duplication of complex, critical logic
essential for safe third-party implementations of custom executors or other scheduling strategies

Unsure:

how this affect performance, if at all?

Requested changes:

I would like to see some more robusts tests to verify that this works, with an emphasis on scenarios that were previously impossible
docs nits

crates/bevy_ecs/src/system/system.rs

tornewuff · 2022-03-07T02:41:15Z

Instead of replacing the new_archetype method, can we just add your update_archetypes method?

Some users (and mocking up tests) might still want to register only specific archetypes.

I'm not sure how common a use case that would be - it seems like it makes the API rather less clear to have both? The interaction between the two isn't really obvious. If there's a general consensus that it's likely to be useful then I can restore it, and just try to describe the behaviour more thoroughly, though.

The other comments all make sense; I agree that better naming would help here, especially if keeping both functions. To propose yet another combination, register_archetype and update_registered_archetypes might work? Though even register_archetype or similar might not be ideal - it's really only notifying the system about the existence of the archetype, since the SystemParam still actually decides whether the archetype is relevant or not.

Since it seems like there's general agreement that this is desirable for the API, I'll see about benchmarking it and adding test coverage.

tornewuff · 2022-03-09T03:12:23Z

Still need to work on tests/docs but wanted to see what folks think of this basic set of benchmarks, which don't show any difference outside of noise for me.

alice-i-cecile · 2022-03-09T03:13:35Z

Some users (and mocking up tests) might still want to register only specific archetypes.

This seem extremely implausible to me. IMO if this is the best argument in favor of keeping it it should be cut for sure.

tornewuff · 2022-03-09T03:13:36Z

(also this is the first time I've actually used github PR code review so am not entirely sure of expectations around resolving comments etc)

alice-i-cecile · 2022-03-09T03:16:24Z

wanted to see what folks think of this basic set of benchmarks, which don't show any difference outside of noise for me.

Could you post the benchmark results in the PR description? It's helpful context for reviewers: no change is good news!

(also this is the first time I've actually used github PR code review so am not entirely sure of expectations around resolving comments etc)

Resolve comments once the changes are addressed :) If there are interesting but unactionable comments / call-outs, keep those around so folks can see those on later review / just before merge.

benches/benches/bevy_ecs/archetype_updates.rs

tornewuff · 2022-03-09T03:33:28Z

Results of the benchmark without the change:

no_archetypes           time:   [39.805 us 39.956 us 40.119 us]                           
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild

added_archetypes        time:   [304.90 us 306.26 us 307.82 us]                             
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe

and with the change:

no_archetypes           time:   [40.220 us 40.335 us 40.454 us]                           
                        change: [+0.2379% +0.6498% +1.0508%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

added_archetypes        time:   [307.27 us 308.21 us 309.15 us]                             
                        change: [+0.1588% +0.7505% +1.3797%] (p = 0.02 < 0.05)
                        Change within noise threshold.
Found 4 outliers among 100 measurements (4.00%)
  1 (1.00%) high mild
  3 (3.00%) high severe

I re-ran this on my giant desktop instead of my ultrabook and there things are less noisy, so it does become statistically significant.

tornewuff · 2022-03-09T03:41:44Z

Hm. Actually it's still pretty noisy on this machine too even with CPU frequency locked.

maniwani · 2022-03-11T02:52:52Z

Thanks for benchmarking, I was hoping there wouldn't be much of a change since we'll still be registering archetypes the same number of times. Good to have some confirmation.

maniwani

Aside from two nits, this looks good to me.

crates/bevy_ecs/src/system/system.rs

crates/bevy_ecs/src/schedule/executor.rs

tornewuff · 2022-03-12T22:43:58Z

So with the updated benchmark here I'm actually seeing performance on added_archetypes with my change included being anything up to 10-15% faster, though the results are still pretty noisy and I'm not sure how to get criterion to generate a nice report with a comparison. So, hopefully it's at least no worse in reality?

I'm still thinking about tests to add here but am pretty happy with the form it's in now; thanks for the continued discussion all!

james7132 · 2022-03-12T23:07:34Z

though the results are still pretty noisy and I'm not sure how to get criterion to generate a nice report with a comparison

critcmp is a pretty common and standard solution here.

tornewuff · 2022-03-12T23:08:10Z

Ah, I have a hypothesis for why the performance might have actually improved for large numbers of new archetypes - previously the update loop in the executor was iterating over all the new archetypes and passing each to every system in turn, but this is now iterating over the systems and having each deal with all the new archetypes, which seems likely to have better locality?

james7132

Overall looks great. Less code, and is potentially faster? Sounds great to me. Just one thing: could you use critcmp as suggested above and provide a solid comparison between the results from before and after?

Add a simple unit test for update_archetype_component_access to ensure that the access is properly updated.

tornewuff · 2022-03-30T01:21:18Z

Well, with the simple unit test I added above it does look like things are working; if folks have suggestions for more things to test I can give it a shot?

While I'm still a little sceptical of the benchmark results being particularly meaningful, it doesn't appear to have broken anything? :)

tornewuff · 2022-04-03T19:08:15Z

OK; I'm considering this done. If folks have other suggestions I can take a look.

crates/bevy_ecs/src/system/mod.rs

Co-authored-by: Alice Cecile <[email protected]>

cart · 2022-04-07T20:13:21Z

bors r+

# Objective - Make it possible to use `System`s outside of the scheduler/executor without having to define logic to track new archetypes and call `System::add_archetype()` for each. ## Solution - Replace `System::add_archetype(&Archetype)` with `System::update_archetypes(&World)`, making systems responsible for tracking their own most recent archetype generation the way that `SystemState` already does. This has minimal (or simplifying) effect on most of the code with the exception of `FunctionSystem`, which must now track the latest `ArchetypeGeneration` it saw instead of relying on the executor to do it. Co-authored-by: Carter Anderson <[email protected]>

bors · 2022-04-07T20:15:51Z

Canceled.

cart · 2022-04-07T20:16:02Z

bors r+

# Objective - Make it possible to use `System`s outside of the scheduler/executor without having to define logic to track new archetypes and call `System::add_archetype()` for each. ## Solution - Replace `System::add_archetype(&Archetype)` with `System::update_archetypes(&World)`, making systems responsible for tracking their own most recent archetype generation the way that `SystemState` already does. This has minimal (or simplifying) effect on most of the code with the exception of `FunctionSystem`, which must now track the latest `ArchetypeGeneration` it saw instead of relying on the executor to do it. Co-authored-by: Carter Anderson <[email protected]>

bors · 2022-04-07T20:28:12Z

Build failed:

build (stable, ubuntu-latest)

cart · 2022-04-07T20:49:01Z

bors r+

# Objective - Make it possible to use `System`s outside of the scheduler/executor without having to define logic to track new archetypes and call `System::add_archetype()` for each. ## Solution - Replace `System::add_archetype(&Archetype)` with `System::update_archetypes(&World)`, making systems responsible for tracking their own most recent archetype generation the way that `SystemState` already does. This has minimal (or simplifying) effect on most of the code with the exception of `FunctionSystem`, which must now track the latest `ArchetypeGeneration` it saw instead of relying on the executor to do it. Co-authored-by: Carter Anderson <[email protected]>

bors · 2022-04-07T21:30:32Z

Pull request successfully merged into main.

Build succeeded:

tornewuff · 2022-04-10T17:04:29Z

Thanks; I didn't intend to leave the println calls in there but since they get swallowed during normal test runs I forgot :)

…#4115) # Objective - Make it possible to use `System`s outside of the scheduler/executor without having to define logic to track new archetypes and call `System::add_archetype()` for each. ## Solution - Replace `System::add_archetype(&Archetype)` with `System::update_archetypes(&World)`, making systems responsible for tracking their own most recent archetype generation the way that `SystemState` already does. This has minimal (or simplifying) effect on most of the code with the exception of `FunctionSystem`, which must now track the latest `ArchetypeGeneration` it saw instead of relying on the executor to do it. Co-authored-by: Carter Anderson <[email protected]>

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 5, 2022

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Mar 5, 2022

hymm reviewed Mar 5, 2022

View reviewed changes

crates/bevy_ecs/src/schedule/executor_parallel.rs Outdated Show resolved Hide resolved

hymm reviewed Mar 5, 2022

View reviewed changes

crates/bevy_ecs/src/system/function_system.rs Outdated Show resolved Hide resolved

alice-i-cecile mentioned this pull request Mar 5, 2022

Fast and correct one-shot systems with a system registry resource #4090

Closed

7 tasks

hymm reviewed Mar 5, 2022

View reviewed changes

alice-i-cecile reviewed Mar 5, 2022

View reviewed changes

crates/bevy_ecs/src/schedule/executor.rs Outdated Show resolved Hide resolved

maniwani reviewed Mar 5, 2022

View reviewed changes

crates/bevy_ecs/src/system/function_system.rs Outdated Show resolved Hide resolved

alice-i-cecile requested changes Mar 5, 2022

View reviewed changes

alice-i-cecile reviewed Mar 5, 2022

View reviewed changes

crates/bevy_ecs/src/system/system.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Mar 9, 2022

View reviewed changes

benches/benches/bevy_ecs/archetype_updates.rs Show resolved Hide resolved

maniwani reviewed Mar 11, 2022

View reviewed changes

crates/bevy_ecs/src/system/system.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/schedule/executor.rs Outdated Show resolved Hide resolved

maniwani approved these changes Mar 13, 2022

View reviewed changes

james7132 reviewed Mar 13, 2022

View reviewed changes

alice-i-cecile approved these changes Mar 13, 2022

View reviewed changes

tornewuff added 2 commits March 29, 2022 21:14

Test update_archetype_component_access.

9e18521

Add a simple unit test for update_archetype_component_access to ensure that the access is properly updated.

fix ecs_bench_suite

7534c12

oops, too many muts

a460fed

maniwani added a commit to maniwani/bevy that referenced this pull request Mar 30, 2022

pull in changes from bevyengine#4115

c7b72f4

This was referenced Mar 30, 2022

System::run does not validate which world it is being run on #4363

Closed

schedule v3 ("stageless") #4391

Closed

fix remaining clippy lints

2a24e21

cart added this to the Bevy 0.7 milestone Apr 4, 2022

cart reviewed Apr 7, 2022

View reviewed changes

crates/bevy_ecs/src/system/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Apr 7, 2022

View reviewed changes

crates/bevy_ecs/src/system/mod.rs Outdated Show resolved Hide resolved

Update crates/bevy_ecs/src/system/mod.rs

4052d18

Co-authored-by: Alice Cecile <[email protected]>

Clippy

4fde02d

More clippy

8379149

bors bot changed the title ~~Make System responsible for updating its own archetypes~~ [Merged by Bors] - Make System responsible for updating its own archetypes Apr 7, 2022

bors bot closed this Apr 7, 2022

tornewuff deleted the update-archetypes branch April 10, 2022 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Make `System` responsible for updating its own archetypes #4115

[Merged by Bors] - Make `System` responsible for updating its own archetypes #4115

tornewuff commented Mar 5, 2022

tornewuff commented Mar 5, 2022

maniwani commented Mar 5, 2022

hymm left a comment

hymm Mar 5, 2022

alice-i-cecile left a comment

tornewuff commented Mar 7, 2022

tornewuff commented Mar 9, 2022

alice-i-cecile commented Mar 9, 2022

tornewuff commented Mar 9, 2022

alice-i-cecile commented Mar 9, 2022

tornewuff commented Mar 9, 2022

tornewuff commented Mar 9, 2022

maniwani commented Mar 11, 2022 •

edited

Loading

maniwani left a comment

tornewuff commented Mar 12, 2022

james7132 commented Mar 12, 2022

tornewuff commented Mar 12, 2022

james7132 left a comment

tornewuff commented Mar 30, 2022

tornewuff commented Apr 3, 2022

cart commented Apr 7, 2022

bors bot commented Apr 7, 2022

cart commented Apr 7, 2022

bors bot commented Apr 7, 2022

cart commented Apr 7, 2022

bors bot commented Apr 7, 2022

tornewuff commented Apr 10, 2022

[Merged by Bors] - Make System responsible for updating its own archetypes #4115

[Merged by Bors] - Make System responsible for updating its own archetypes #4115

Conversation

tornewuff commented Mar 5, 2022

Objective

Solution

tornewuff commented Mar 5, 2022

maniwani commented Mar 5, 2022

hymm left a comment

Choose a reason for hiding this comment

hymm Mar 5, 2022

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

tornewuff commented Mar 7, 2022

tornewuff commented Mar 9, 2022

alice-i-cecile commented Mar 9, 2022

tornewuff commented Mar 9, 2022

alice-i-cecile commented Mar 9, 2022

tornewuff commented Mar 9, 2022

tornewuff commented Mar 9, 2022

maniwani commented Mar 11, 2022 • edited Loading

maniwani left a comment

Choose a reason for hiding this comment

tornewuff commented Mar 12, 2022

james7132 commented Mar 12, 2022

tornewuff commented Mar 12, 2022

james7132 left a comment

Choose a reason for hiding this comment

tornewuff commented Mar 30, 2022

tornewuff commented Apr 3, 2022

cart commented Apr 7, 2022

bors bot commented Apr 7, 2022

cart commented Apr 7, 2022

bors bot commented Apr 7, 2022

cart commented Apr 7, 2022

bors bot commented Apr 7, 2022

tornewuff commented Apr 10, 2022

[Merged by Bors] - Make `System` responsible for updating its own archetypes #4115

[Merged by Bors] - Make `System` responsible for updating its own archetypes #4115

maniwani commented Mar 11, 2022 •

edited

Loading