Opt out change detection #6659

james7132 · 2022-11-17T07:42:44Z

Objective

Allow users to opt out of change detection at the type level. Change detection comes with a performance cost on every mutative query. Disabling change detection at the type level can act as a performance optimization for all queries it's involved in.

Solution

Add the constant CHANGE_DETECTION_ENABLED to Component and have it default to true. Extend the Component derive macro to allow easy configuration of this constant. Use this constant to return constant false results to change detection queries.

#[derive(Component)]
#[component(change_detection = false)]
pub struct ChangeDetectionless;

Disabling change detection will make Mut::is_added, Mut::is_changed, Added<T> queries, and Changed<T> all to return false. Any query using the filters will return empty results.

ZSTs will always make Mut::set_changed a no-op, even if change detection is enabled.

Follow-up

Disable change detection on all render world types.
Disable storage based on this constant (if possible without incurring a performance penalty).

james7132 · 2022-11-17T09:05:54Z

Discussed the need for benchmarks with @cart on Discord:

[12:12 AM] cart: Makes sense. I also just noticed that this doesn't (yet) disable storage for these types, which was one of my big motivators for benches. You added that to the followup section, which does make sense.

alice-i-cecile · 2022-11-17T13:41:01Z

ZSTs will always make Mut::set_changed a no-op, even if change detection is enabled.

Added a Breaking Change label due to this. I think this is the right course of action, but I know at least @TheRawMeatball takes advantage of this quirk.

Migration Guide

Components (and resources) with no data can no longer be manually set as changed. If you were taking advantage of this quirk, store internal data, or swap to events.

alice-i-cecile · 2022-11-17T13:42:17Z

crates/bevy_ecs/src/component.rs

+/// # Disabling Change Detection
+///
+/// By default, Bevy will track every mutative access made to a given component.
+/// This may incur a performance cost on types that do not need it.


Suggested change

/// This may incur a performance cost on types that do not need it.

/// This may incur a performance cost (primarily in terms of memory usage) on types that do not need it.

Not sure I agree with this when it's currently blocking autovectorization, as seen in #6547. It's both a CPU and a memory cost in that case.

crates/bevy_ecs/src/component.rs

alice-i-cecile

Code looks correct, and the docs are solid. In addition to my small comments, I feel pretty firmly that we should automatically opt all ZSTs out of change detection, with no override (or at least, no override with the derive macro). The macro should fail to compile (or panic at startup if that's not possible) if we're discussing a struct with no fields.

That behavior is unintuitive and a memory usage footgun: allowing it results in more complexity than it's worth, especially if set_changed is hard coded to a no-op.

james7132 · 2022-11-17T16:57:00Z

I was thinking people could use Added<T> to find when they've been added. Seemed like a decent use case to me.

Resources didn't have this implemented. I'll go ahead and add it.

alice-i-cecile · 2022-11-17T18:01:50Z

I was thinking people could use Added to find when they've been added. Seemed like a decent use case to me.

Ah, this makes sense to me. Can we control added and changed separately then?

JMS55 · 2022-11-17T19:25:03Z

Disclaimer: I'm not very familiar with the ECS side of bevy.

Does it make sense that change detection is opt-out by default? I would've thought it would be opt-in, so that you don't pay for it unless you actually need it. I could see the worry about forgetting to opt-in and silently not working, but I dislike that all mutable queries pay a performance cost (although, how large in practice?). Thoughts?

alice-i-cecile · 2022-11-17T19:41:39Z

I feel pretty strongly that this should be opt-out by default:

When prototyping and learning, you don't care about performance overheads
These overheads are genuinely pretty small.
The failure mode is pretty insidious.
Ecosystem crates should have change detection on for virtually everything, because who knows what users might need it for.

That said, I think that future work should include a schedule analyzer that checks for uses of added / changed filters, and tells you what you could turn off.

james7132 · 2022-11-17T20:08:26Z

Does it make sense that change detection is opt-out by default? I would've thought it would be opt-in, so that you don't pay for it unless you actually need it. I could see the worry about forgetting to opt-in and silently not working, but I dislike that all mutable queries pay a performance cost (although, how large in practice?). Thoughts?

Currently, it's 8 bytes per component in memory, about the same as a usize or a pointer. For anything larger than 8 bytes, using change detection as a filter is almost always a performance benefit when reading, with a small and vectorizable cost on write. As a feature, it's also critical to diffing network states and minimizing work done for retained state extraction for rendering.

However, there are cases, like in the current rendering implementation, where we clear the entire World every frame. In these cases, change detection only adds extra unnecessary overhead. Types that are also smaller than 8 bytes (i.e. Visibility) may not benefit perf wise from using change detection. Overall these cases are far and few in between, so having a common sense default with a valid escape hatch seems like a better solution than to have it disabled by default. Though it runs against the Rust ideology as it isn't a zero-cost abstraction.

These overheads are genuinely pretty small.

I agree with most of what is said here, but this might be (very) wrong. In the general case, change detection is a fairly minimal overhead, but without #6547, change detection is a hard block to autovectorization for all forms of query iteration (see these results). This is a massive perf downgrade where it would otherwise be possible. Even with #6547, change detection may be a blocker between enabling it for Query::iter where it's otherwise possible for Query::for_each .

The failure mode is pretty insidious.

We could make it opt-in and just spit out a warning when using change detection APIs (if we had custom lints).

alice-i-cecile · 2022-11-17T20:43:06Z

We could make it opt-in and just spit out a warning when using change detection APIs (if we had custom lints).

Fair! We could also use schedule analysis, or a debug-only error macro?

JoJoJet

Just some minor nits, still working on a real review.

JoJoJet · 2022-12-04T21:59:13Z

crates/bevy_ecs/macros/src/component.rs

        }
    })
 }

 pub const COMPONENT: Symbol = Symbol("component");
+pub const RESOURCE: Symbol = Symbol("resource");
+pub const CHANGED_DETECTION: Symbol = Symbol("change_detection");


Suggested change

pub const CHANGED_DETECTION: Symbol = Symbol("change_detection");

pub const CHANGE_DETECTION: Symbol = Symbol("change_detection");

JoJoJet · 2022-12-04T21:59:31Z

crates/bevy_ecs/macros/src/component.rs

@@ -93,6 +118,17 @@ fn parse_component_attr(ast: &DeriveInput) -> Result<Attrs> {
                    }
                };
            }
+            Meta(NameValue(m)) if m.path == CHANGED_DETECTION => {


Suggested change

Meta(NameValue(m)) if m.path == CHANGED_DETECTION => {

Meta(NameValue(m)) if m.path == CHANGE_DETECTION => {

JoJoJet · 2022-12-04T21:59:53Z

crates/bevy_ecs/macros/src/component.rs

+            NestedMeta::{Lit, Meta},
+        };
+        match meta {
+            Meta(NameValue(m)) if m.path == CHANGED_DETECTION => {


Suggested change

Meta(NameValue(m)) if m.path == CHANGED_DETECTION => {

Meta(NameValue(m)) if m.path == CHANGE_DETECTION => {

JoJoJet · 2022-12-04T22:10:46Z

crates/bevy_ecs/src/component.rs

@@ -111,7 +127,12 @@ use std::{
 /// [orphan rule]: https://doc.rust-lang.org/book/ch10-02-traits.html#implementing-a-trait-on-a-type
 /// [newtype pattern]: https://doc.rust-lang.org/book/ch19-03-advanced-traits.html#using-the-newtype-pattern-to-implement-external-traits-on-external-types
 pub trait Component: Send + Sync + 'static {
+    type WriteFetch<'a>: ComponentMut<'a> + DetectChanges<Inner = Self> + DerefMut<Target = Self>;


I don't think Fetch is the right term to use here, since this type is the actual item being returned, not an intermediate value.

JoJoJet · 2022-12-04T22:36:33Z

crates/bevy_ecs/src/change_detection.rs

 impl_methods!(Mut<'a, T>, T,);
 impl_debug!(Mut<'a, T>,);

+impl<'a, T: ?Sized> DetectChanges for &'a mut T {


This impl seems like a bit of a footgun. If you have a variable of type &mut Mut<T>, then it will implicitly use this no-op impl any time you call .is_changed().

inodentry · 2022-12-09T15:02:00Z

@BoxyUwU maybe you want to look at this PR? Maybe you'd have valuable input?

JoJoJet · 2022-12-09T22:36:17Z

Boxy, James, and I discussed this PR in this discord thread, where we explored some options for where to take this feature: https://discord.com/channels/691052431525675048/749335865876021248/1049094784096141363

then we went back on topic here: https://discord.com/channels/691052431525675048/749335865876021248/1049104605042184283

james7132 · 2022-12-09T23:06:43Z

To summarize that discussion, the best option right now might be to use the same "soft constant" approach used by #6800, where we rely on heavy inlining to optimize out branches based on a supposedly runtime-stored value (enabled/disabled) sourced from a constant (T:CHANGE_DETECTION_ENABLED). This by far has the least churn while delivering on the same final goal. It can also transitively carry over into MutUntyped as well. So long as people aren't writing Mut<T> to some heap memory, it should have the same performance profile as it has now.

cart · 2022-12-16T02:22:36Z

Another option that I don't fully believe in but might be worth discussing now (not trying to derail): maybe we make change detection 100% runtime configuration? Pretty sure this has been proposed in the past. I might even have shot the idea down in Bevy's early days 😅

Mut<T> becomes an enum (Mut::ChangeDetected vs Mut::Normal), or stores a flag (if that makes more sense). If someone uses a Changed<T> query, change detection is automatically enabled for all components. We could also support explicit global config to "force" a certain behavor (ex: ChangeDetectionPolicy { AutoDetect, AlwaysOn, AlwaysOff, AutoDetectButNeverTurnOffOnceEnabled }). If the settings change (either because a system is added or a user takes action), we invalidate QueryStates that are affected and adjust our storage accordingly (ex: allocate or deallocate change detection space in the relevant tables). This should be a rare op.

Some pros:

Switching change detection on and off is a non-breaking change for systems, as the type is Mut no matter what
We are "automatically optimal" from a "where is change detection enabled" perspective. Library authors don't need to make the call between perf and flexibility.
Like a static impl (like the one in this pr), we have the option to reduce memory allocations, reads, and writes when change detection is disabled

Some cons:

We add a branch to all mutable derefs of Mut. No clue how this compares to the cost of change detection being enabled globally. We'd probably want to compare impls.
We no longer have the ability to make &mut T queries resolve to &mut T (this is both a pro and a con. pro: switching between the two types in a static impl is a breaking change, so consistency is good. con: &mut T is clearer, more efficient, and easier to use, so losing that is sad)
Adding / removing as system with change detection at runtime could be expensive (ex: if there are already thousands of Monster entities and we enable change detection for monsters, that would be a "big" implicit allocation). In practice adding/removing systems is already an expensive operation and I think change detection state will almost always be determined once at startup.
Hard to put this genie back in the bottle. If we later choose to switch back to a static configuration, that would probably be massively disruptive to the community.

I think the biggest concern is performance, but if we can prove its actually faster in the majority of cases, that would be pretty sweet!

james7132 · 2022-12-16T02:41:31Z

We add a branch to all mutable derefs of Mut. No clue how this compares to the cost of change detection being enabled globally. We'd probably want to compare impls.

I'm pretty sure this will break the vectorization gains made in #6547 since the compiler cannot prove at compile time whether or not it's going to branch or not. If possible I would really like this a compile time decision as the branch could easily be more expensive as the write out to the tick buffer.

What is described above (a singular enabled/disabled flag) is probably the least intrusive way to approach this. However, if it's not sourced from a constant but rather a runtime field as proposed, the compiler cannot optimize out the branch, and will break many of the optimizations we've been trying to keep.

BoxyUwU · 2022-12-16T04:31:43Z

It seems like whether change detection is set to true/false is a check that can be done outside of a for loop since its not going to change from one next call to the next next. 🤷‍♀️ as to whether compiler is smart enough to do that though

cart · 2022-12-16T21:05:25Z

I'm pretty sure this will break the vectorization gains made in #6547 since the compiler cannot prove at compile time whether or not it's going to branch or not. If possible I would really like this a compile time decision as the branch could easily be more expensive as the write out to the tick buffer.

Maybe we could support both modes? A compile time "change detection settings" const enum with Enabled, Disabled, and DetectedAtRuntime.

Then for high traffic types (guaranteed to use change detection) like Transform we could force it to "enabled" for maximum perf.

cart · 2022-12-16T21:07:54Z

It seems worthwhile to impl both approaches and compare. And we could probably just start with (and merge) the compile-time variant, then tack on the DetectedAtRuntime option once we find the time to do the impl.

cBournhonesque · 2023-01-18T07:42:37Z

crates/bevy_ecs/src/change_detection.rs

@@ -23,6 +23,17 @@ pub const CHECK_TICK_THRESHOLD: u32 = 518_400_000;
 /// Changes stop being detected once they become this old.
 pub const MAX_CHANGE_AGE: u32 = u32::MAX - (2 * CHECK_TICK_THRESHOLD - 1);

+pub trait ComponentMut<'a>: DetectChanges {


Maybe a quick doc explaining that this trait is just to add the const ENABLED: bool to either &mut T or Mut<T> could be useful

cBournhonesque

I've seen some people complain on slack about the 'fake' implementation of DetectChanges for &mut T, where is_changed and is_added are set to true; because it can be misleading/confusing.

Ultimately I think the problem is that ComponentMut (whether or not we return Mut<T> or &mut T) should be decoupled from DetectChanges?

I tried updating your PR to make it work, like this: https://github.com/james7132/bevy/pull/5/files

(it looks like WriteFetch never makes use of the DetectChanges bound, because WriteFetch is only used for ChangeTrackers<T>, which has its own change_ticks)

- Fixes bevyengine#7732 - Based extensively off bevyengine#6659 - Completely broken as I didn't change any of the ~100 or so assumptions that &T == &T while it now is Ref<T>

alice-i-cecile · 2023-06-19T16:02:41Z

This doesn't look like it'll be ready for the 0.11 milestone <3

inodentry · 2023-11-06T12:40:02Z

So, what is the status here? This PR missed 0.11 and now also 0.12. I think this work is valuable. Could we try to revisit this for 0.13?

richchurcher · 2024-10-20T02:44:01Z

Backlog cleanup: obviously still needed, but seems like more design required (checked in with Discord here for context). Note existing tracking issue #4882, possibly others.

james7132 added 9 commits November 16, 2022 22:20

Add Component::CHANGE_DETECTION_ENABLED

e272d94

Extend Component derive macro

c7b2d7f

Update Component docs

c7e2c2d

Add test for the macro changes

fa32533

Move ZST size check

dc52dfd

Update docs

9c3e2d7

Fix test

fa4cba4

Fix CI

8043968

Updated Added and Changed filters

cc88d5d

james7132 added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times labels Nov 17, 2022

james7132 requested review from alice-i-cecile and maniwani November 17, 2022 07:47

cart added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Nov 17, 2022

james7132 removed the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Nov 17, 2022

alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Nov 17, 2022

alice-i-cecile reviewed Nov 17, 2022

View reviewed changes

crates/bevy_ecs/src/component.rs Show resolved Hide resolved

alice-i-cecile requested changes Nov 17, 2022

View reviewed changes

james7132 added 2 commits November 17, 2022 09:42

Add the same changes to Resource

51e5bc5

Move test to change_detection.rs

971024c

Add (and fix) ZST change detection test

df3c6db

alice-i-cecile self-requested a review December 4, 2022 14:36

JoJoJet reviewed Dec 4, 2022

View reviewed changes

inodentry requested a review from BoxyUwU December 9, 2022 15:01

cBournhonesque mentioned this pull request Jan 17, 2023

PartialEq and Eq based change detection #5618

Open

cBournhonesque reviewed Jan 18, 2023

View reviewed changes

cBournhonesque mentioned this pull request Jan 18, 2023

Add "PartialEq" mode for change detection #7270

Closed

Guvante mentioned this pull request Jan 23, 2023

Make Query<&T> return Ref<T> #7322

Open

Guvante mentioned this pull request Jan 23, 2023

Add change detection that swaps between &/&mut and Ref/Mut when disabled/enabled #7344

Closed

Guvante mentioned this pull request Feb 4, 2023

Basic concept for customizing Component reference type #7499

Draft

james7132 added this to the 0.11 milestone Feb 27, 2023

alice-i-cecile removed this from the 0.11 milestone Jun 19, 2023

ItsDoot added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Aug 19, 2024

richchurcher closed this Oct 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Opt out change detection #6659

Opt out change detection #6659

james7132 commented Nov 17, 2022 •

edited

Loading

james7132 commented Nov 17, 2022

alice-i-cecile commented Nov 17, 2022

alice-i-cecile Nov 17, 2022

james7132 Nov 17, 2022

alice-i-cecile left a comment •

edited

Loading

james7132 commented Nov 17, 2022 •

edited

Loading

alice-i-cecile commented Nov 17, 2022

JMS55 commented Nov 17, 2022

alice-i-cecile commented Nov 17, 2022

james7132 commented Nov 17, 2022

alice-i-cecile commented Nov 17, 2022

JoJoJet left a comment

JoJoJet Dec 4, 2022

JoJoJet Dec 4, 2022

JoJoJet Dec 4, 2022

JoJoJet Dec 4, 2022

JoJoJet Dec 4, 2022

inodentry commented Dec 9, 2022

JoJoJet commented Dec 9, 2022

james7132 commented Dec 9, 2022 •

edited

Loading

cart commented Dec 16, 2022 •

edited

Loading

james7132 commented Dec 16, 2022 •

edited

Loading

BoxyUwU commented Dec 16, 2022

cart commented Dec 16, 2022

cart commented Dec 16, 2022

cBournhonesque Jan 18, 2023

cBournhonesque left a comment •

edited

Loading

alice-i-cecile commented Jun 19, 2023

inodentry commented Nov 6, 2023

richchurcher commented Oct 20, 2024

	/// This may incur a performance cost on types that do not need it.
	/// This may incur a performance cost (primarily in terms of memory usage) on types that do not need it.

	pub const CHANGED_DETECTION: Symbol = Symbol("change_detection");
	pub const CHANGE_DETECTION: Symbol = Symbol("change_detection");

	Meta(NameValue(m)) if m.path == CHANGED_DETECTION => {
	Meta(NameValue(m)) if m.path == CHANGE_DETECTION => {

Opt out change detection #6659

Opt out change detection #6659

Conversation

james7132 commented Nov 17, 2022 • edited Loading

Objective

Solution

Follow-up

james7132 commented Nov 17, 2022

alice-i-cecile commented Nov 17, 2022

Migration Guide

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile left a comment • edited Loading

Choose a reason for hiding this comment

james7132 commented Nov 17, 2022 • edited Loading

alice-i-cecile commented Nov 17, 2022

JMS55 commented Nov 17, 2022

alice-i-cecile commented Nov 17, 2022

james7132 commented Nov 17, 2022

alice-i-cecile commented Nov 17, 2022

JoJoJet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inodentry commented Dec 9, 2022

JoJoJet commented Dec 9, 2022

james7132 commented Dec 9, 2022 • edited Loading

cart commented Dec 16, 2022 • edited Loading

james7132 commented Dec 16, 2022 • edited Loading

BoxyUwU commented Dec 16, 2022

cart commented Dec 16, 2022

cart commented Dec 16, 2022

Choose a reason for hiding this comment

cBournhonesque left a comment • edited Loading

Choose a reason for hiding this comment

alice-i-cecile commented Jun 19, 2023

inodentry commented Nov 6, 2023

richchurcher commented Oct 20, 2024

james7132 commented Nov 17, 2022 •

edited

Loading

alice-i-cecile left a comment •

edited

Loading

james7132 commented Nov 17, 2022 •

edited

Loading

james7132 commented Dec 9, 2022 •

edited

Loading

cart commented Dec 16, 2022 •

edited

Loading

james7132 commented Dec 16, 2022 •

edited

Loading

cBournhonesque left a comment •

edited

Loading