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

Changed doesn't explain any potential drawback #3082

Closed
djeedai opened this issue Nov 6, 2021 · 13 comments
Closed

Changed doesn't explain any potential drawback #3082

djeedai opened this issue Nov 6, 2021 · 13 comments
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation

Comments

@djeedai
Copy link
Contributor

djeedai commented Nov 6, 2021

How can Bevy's documentation be improved?

https://docs.rs/bevy/0.5.0/bevy/ecs/query/struct.Changed.html

The docs mention this is a performance optimization to iterate over changed entities/components only. There's no mention of what the cost of maintaining such information is. Surely this doesn't come "for free"; there must be some caching somewhere to compare with the previous frame's version and be able to tell. What's the cost? Can this be used on many different single-components (like, should I use Changed<MyType> for most of my own types, even if used once or twice only)? Or is it more beneficial on a single component with many instances (like the Changed<Transform> example? Is there a minimum order of magnitude number below which Changed is prohibitive compared to just iterating over all existing items without any caching?

I checked the docs on main too, and it's no better. It just mentions ChangeTrackers in addition; I'm not even understanding the difference between ChangeTrackers and Changed.

@djeedai djeedai added C-Docs An addition or correction to our documentation S-Needs-Triage This issue needs to be labelled labels Nov 6, 2021
@bjorn3
Copy link
Contributor

bjorn3 commented Nov 6, 2021

Every time you mutate a component, the "tick" at which it changed is stored. Changed<T> compares the stored "tick" of the component with the "tick" at which the system ran last. If it is later, this means that the component changed since the last time. This "tick" identifier is a single u32, so a 4 byte overhead per component per entity.

@djeedai
Copy link
Contributor Author

djeedai commented Nov 6, 2021

Unless there's another alternative I'm not seeing, you can't know if a system mutated a component unless either 1) you assume that any ResMut<> query means a mutation (conservative, but has false positives), or 2) you do a diff of the pre- and post- version of the component. So either it's 1) and Changed is less powerful that it says, or it's 2) and there's a cost not mentioned. Isn't it?

@cart
Copy link
Member

cart commented Nov 7, 2021

You're missing option 3: utilize rust's DerefMut implementation to automatically overwrite the tick whenever a component or resource is accessed mutably. This means just querying for these things isn't enough to trigger a mutation. You must actually mutate (according to the rust compiler). Of course, you could mutate a component by setting to the exact same value, but I think this is the correct tradeoff relative to actually diffing components.

We need recording a change to be very cheap (because mutations can happen frequently), so its literally just setting the tick value. We don't collect a list of mutated entities because allocating on mutation to populate a list (and de-duplicating the list) would be prohibitively expensive. We also don't do actual-value comparisons for the same reason. The tradeoff we landed on is that iterating Changed components checks every component's tick for the given type: we iterate every component of the given type ... not just the changed ones. This happens internally and users just receive actual Changed components.

@cart
Copy link
Member

cart commented Nov 7, 2021

Ah the doc in question is wrong though:

This filter is useful for synchronizing components, and as a performance optimization as it means that the query contains fewer items for a system to iterate over.

I think something was lost in translation here by the doc author: Changed can be used as a way to optimize expensive operations by only doing them for changed components (ex: only updating the GlobalTransform when the Transform changes). The act of iterating changed components can actually be slightly more expensive than iterating all components of a given type because we need to check the tick in addition to retrieving the component for changed entities (although it might be faster in some cases because we dont actually need to retrieve unchanged components).

@cart cart added this to the Bevy 0.6 milestone Nov 7, 2021
@cart cart removed the S-Needs-Triage This issue needs to be labelled label Nov 7, 2021
@djeedai
Copy link
Contributor Author

djeedai commented Nov 7, 2021

Ok thanks for the detailed explanation. That's 1b) really, under the general umbrella of "assume modified on mutating getter". Don't get me wrong, that's absolutely the right thing to do for sure, but the details should probably be explained in the docs to call out the potential false positive (which may matter I'm some cases) and the extra work you mention. I'll try to make a proposal PR.

@cart
Copy link
Member

cart commented Nov 7, 2021

Sounds good to me!

@djeedai
Copy link
Contributor Author

djeedai commented Nov 7, 2021

@cart the current docs says:

Filter that retrieves components of type T that have been changed since the last execution of this system.

Is that really the case?! I'd assume if system A mutates a component, system B filtering with Changed<T> will observe that change in the same frame if it runs after system A, no? The change detection is per component, not per system if I understand correctly how it works (ComponentTicks).

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 7, 2021

A system doesn't have to run every frame. The nice thing about bevy's change detection is that skipping a system won't break change detection. It will report it the next time the system runs. That is why every system has it's own "tick" indicating when it ran the last time that is compared with the component "tick" indicating when it was changed the last time.

djeedai added a commit to djeedai/bevy that referenced this issue Nov 7, 2021
Fix the documentation of the `Changed` filter to detail its mutating
detection functioning, and explain the advantages and drawbacks of using
it.

Bug: bevyengine#3082
@djeedai
Copy link
Contributor Author

djeedai commented Nov 7, 2021

Thanks, but that's not exactly the question. Sure, it's nice that if a system doesn't tick a frame, Changed still works. But I'm more interested in confirming that if e.g. 2 different systems are changing a component then Changed will be "triggered" on both changes. And also that if a system changes a component then a second system running after it in the same frame will also see the changed component. The docs currently saying "since the last execution of this system" seem to imply that Changed is a per-component but also per-system change detection; this is not the case as I understand, whatever the source of the change Changed will record it equally.

@djeedai
Copy link
Contributor Author

djeedai commented Nov 7, 2021

Opened PR #3084, we can iterate on proper wording there.

@cart
Copy link
Member

cart commented Nov 7, 2021

Thanks, but that's not exactly the question. Sure, it's nice that if a system doesn't tick a frame, Changed still works. But I'm more interested in confirming that if e.g. 2 different systems are changing a component then Changed will be "triggered" on both changes.

I think @bjorn3 did pretty much answer your question by saying that each system has its own tick, which is compared against the component tick. Change detection is per system. If a change (which happened anywhere / in any system) occurred since the last time a system ran, the change will show up in that system's Change queries.

@cart cart removed this from the Bevy 0.6 milestone Dec 18, 2021
@cart
Copy link
Member

cart commented Dec 18, 2021

Removed from the milestone because this isn't really needed for the release and we're nearing the cutoff point. If it makes it in, thats cool though :)

bors bot pushed a commit that referenced this issue May 9, 2022
## Objective

- ~~Make absurdly long-lived changes stay detectable for even longer (without leveling up to `u64`).~~
- Give all changes a consistent maximum lifespan.
- Improve code clarity.

## Solution

- ~~Increase the frequency of `check_tick` scans to increase the oldest reliably-detectable change.~~
(Deferred until we can benchmark the cost of a scan.)
- Ignore changes older than the maximum reliably-detectable age.
- General refactoring—name the constants, use them everywhere, and update the docs.
- Update test cases to check for the specified behavior.

## Related

This PR addresses (at least partially) the concerns raised in:

- #3071
- #3082 (and associated PR #3084)

## Background

- #1471

Given the minimum interval between `check_ticks` scans, `N`, the oldest reliably-detectable change is `u32::MAX - (2 * N - 1)` (or `MAX_CHANGE_AGE`). Reducing `N` from ~530 million (current value) to something like ~2 million would extend the lifetime of changes by a billion.

| minimum `check_ticks` interval | oldest reliably-detectable change  | usable % of `u32::MAX` |
| --- | --- | --- |
| `u32::MAX / 8`  (536,870,911) | `(u32::MAX / 4) * 3` | 75.0% |
| `2_000_000` | `u32::MAX - 3_999_999` | 99.9% |

Similarly, changes are still allowed to be between `MAX_CHANGE_AGE`-old and `u32::MAX`-old in the interim between `check_tick` scans. While we prevent their age from overflowing, the test to detect changes still compares raw values. This makes failure ultimately unreliable, since when ancient changes stop being detected varies depending on when the next scan occurs.

## Open Question

Currently, systems and system states are incorrectly initialized with their `last_change_tick` set to `0`, which doesn't handle wraparound correctly.

For consistent behavior, they should either be initialized to the world's `last_change_tick` (and detect no changes) or to `MAX_CHANGE_AGE` behind the world's current `change_tick` (and detect everything as a change). I've currently gone with the latter since that was closer to the existing behavior.

## Follow-up Work

(Edited: entire section)

We haven't actually profiled how long a `check_ticks` scan takes on a "large" `World` , so we don't know if it's safe to increase their frequency. However, we are currently relying on play sessions not lasting long enough to trigger a scan and apps not having enough entities/archetypes for it to be "expensive" (our assumption). That isn't a real solution. (Either scanning never costs enough to impact frame times or we provide an option to use `u64` change ticks. Nobody will accept random hiccups.)

To further extend the lifetime of changes, we actually only need to increment the world tick if a system has `Fetch: !ReadOnlySystemParamFetch`. The behavior will be identical because all writes are sequenced, but I'm not sure how to implement that in a way that the compiler can optimize the branch out.

Also, since having no false positives depends on a `check_ticks` scan running at least every `2 * N - 1` ticks, a `last_check_tick` should also be stored in the `World` so that any lull in system execution (like a command flush) could trigger a scan if needed. To be completely robust, all the systems initialized on the world should be scanned, not just those in the current stage.
bors bot pushed a commit that referenced this issue May 9, 2022
## Objective

- ~~Make absurdly long-lived changes stay detectable for even longer (without leveling up to `u64`).~~
- Give all changes a consistent maximum lifespan.
- Improve code clarity.

## Solution

- ~~Increase the frequency of `check_tick` scans to increase the oldest reliably-detectable change.~~
(Deferred until we can benchmark the cost of a scan.)
- Ignore changes older than the maximum reliably-detectable age.
- General refactoring—name the constants, use them everywhere, and update the docs.
- Update test cases to check for the specified behavior.

## Related

This PR addresses (at least partially) the concerns raised in:

- #3071
- #3082 (and associated PR #3084)

## Background

- #1471

Given the minimum interval between `check_ticks` scans, `N`, the oldest reliably-detectable change is `u32::MAX - (2 * N - 1)` (or `MAX_CHANGE_AGE`). Reducing `N` from ~530 million (current value) to something like ~2 million would extend the lifetime of changes by a billion.

| minimum `check_ticks` interval | oldest reliably-detectable change  | usable % of `u32::MAX` |
| --- | --- | --- |
| `u32::MAX / 8`  (536,870,911) | `(u32::MAX / 4) * 3` | 75.0% |
| `2_000_000` | `u32::MAX - 3_999_999` | 99.9% |

Similarly, changes are still allowed to be between `MAX_CHANGE_AGE`-old and `u32::MAX`-old in the interim between `check_tick` scans. While we prevent their age from overflowing, the test to detect changes still compares raw values. This makes failure ultimately unreliable, since when ancient changes stop being detected varies depending on when the next scan occurs.

## Open Question

Currently, systems and system states are incorrectly initialized with their `last_change_tick` set to `0`, which doesn't handle wraparound correctly.

For consistent behavior, they should either be initialized to the world's `last_change_tick` (and detect no changes) or to `MAX_CHANGE_AGE` behind the world's current `change_tick` (and detect everything as a change). I've currently gone with the latter since that was closer to the existing behavior.

## Follow-up Work

(Edited: entire section)

We haven't actually profiled how long a `check_ticks` scan takes on a "large" `World` , so we don't know if it's safe to increase their frequency. However, we are currently relying on play sessions not lasting long enough to trigger a scan and apps not having enough entities/archetypes for it to be "expensive" (our assumption). That isn't a real solution. (Either scanning never costs enough to impact frame times or we provide an option to use `u64` change ticks. Nobody will accept random hiccups.)

To further extend the lifetime of changes, we actually only need to increment the world tick if a system has `Fetch: !ReadOnlySystemParamFetch`. The behavior will be identical because all writes are sequenced, but I'm not sure how to implement that in a way that the compiler can optimize the branch out.

Also, since having no false positives depends on a `check_ticks` scan running at least every `2 * N - 1` ticks, a `last_check_tick` should also be stored in the `World` so that any lull in system execution (like a command flush) could trigger a scan if needed. To be completely robust, all the systems initialized on the world should be scanned, not just those in the current stage.
robtfm pushed a commit to robtfm/bevy that referenced this issue May 10, 2022
## Objective

- ~~Make absurdly long-lived changes stay detectable for even longer (without leveling up to `u64`).~~
- Give all changes a consistent maximum lifespan.
- Improve code clarity.

## Solution

- ~~Increase the frequency of `check_tick` scans to increase the oldest reliably-detectable change.~~
(Deferred until we can benchmark the cost of a scan.)
- Ignore changes older than the maximum reliably-detectable age.
- General refactoring—name the constants, use them everywhere, and update the docs.
- Update test cases to check for the specified behavior.

## Related

This PR addresses (at least partially) the concerns raised in:

- bevyengine#3071
- bevyengine#3082 (and associated PR bevyengine#3084)

## Background

- bevyengine#1471

Given the minimum interval between `check_ticks` scans, `N`, the oldest reliably-detectable change is `u32::MAX - (2 * N - 1)` (or `MAX_CHANGE_AGE`). Reducing `N` from ~530 million (current value) to something like ~2 million would extend the lifetime of changes by a billion.

| minimum `check_ticks` interval | oldest reliably-detectable change  | usable % of `u32::MAX` |
| --- | --- | --- |
| `u32::MAX / 8`  (536,870,911) | `(u32::MAX / 4) * 3` | 75.0% |
| `2_000_000` | `u32::MAX - 3_999_999` | 99.9% |

Similarly, changes are still allowed to be between `MAX_CHANGE_AGE`-old and `u32::MAX`-old in the interim between `check_tick` scans. While we prevent their age from overflowing, the test to detect changes still compares raw values. This makes failure ultimately unreliable, since when ancient changes stop being detected varies depending on when the next scan occurs.

## Open Question

Currently, systems and system states are incorrectly initialized with their `last_change_tick` set to `0`, which doesn't handle wraparound correctly.

For consistent behavior, they should either be initialized to the world's `last_change_tick` (and detect no changes) or to `MAX_CHANGE_AGE` behind the world's current `change_tick` (and detect everything as a change). I've currently gone with the latter since that was closer to the existing behavior.

## Follow-up Work

(Edited: entire section)

We haven't actually profiled how long a `check_ticks` scan takes on a "large" `World` , so we don't know if it's safe to increase their frequency. However, we are currently relying on play sessions not lasting long enough to trigger a scan and apps not having enough entities/archetypes for it to be "expensive" (our assumption). That isn't a real solution. (Either scanning never costs enough to impact frame times or we provide an option to use `u64` change ticks. Nobody will accept random hiccups.)

To further extend the lifetime of changes, we actually only need to increment the world tick if a system has `Fetch: !ReadOnlySystemParamFetch`. The behavior will be identical because all writes are sequenced, but I'm not sure how to implement that in a way that the compiler can optimize the branch out.

Also, since having no false positives depends on a `check_ticks` scan running at least every `2 * N - 1` ticks, a `last_check_tick` should also be stored in the `World` so that any lull in system execution (like a command flush) could trigger a scan if needed. To be completely robust, all the systems initialized on the world should be scanned, not just those in the current stage.
exjam pushed a commit to exjam/bevy that referenced this issue May 22, 2022
## Objective

- ~~Make absurdly long-lived changes stay detectable for even longer (without leveling up to `u64`).~~
- Give all changes a consistent maximum lifespan.
- Improve code clarity.

## Solution

- ~~Increase the frequency of `check_tick` scans to increase the oldest reliably-detectable change.~~
(Deferred until we can benchmark the cost of a scan.)
- Ignore changes older than the maximum reliably-detectable age.
- General refactoring—name the constants, use them everywhere, and update the docs.
- Update test cases to check for the specified behavior.

## Related

This PR addresses (at least partially) the concerns raised in:

- bevyengine#3071
- bevyengine#3082 (and associated PR bevyengine#3084)

## Background

- bevyengine#1471

Given the minimum interval between `check_ticks` scans, `N`, the oldest reliably-detectable change is `u32::MAX - (2 * N - 1)` (or `MAX_CHANGE_AGE`). Reducing `N` from ~530 million (current value) to something like ~2 million would extend the lifetime of changes by a billion.

| minimum `check_ticks` interval | oldest reliably-detectable change  | usable % of `u32::MAX` |
| --- | --- | --- |
| `u32::MAX / 8`  (536,870,911) | `(u32::MAX / 4) * 3` | 75.0% |
| `2_000_000` | `u32::MAX - 3_999_999` | 99.9% |

Similarly, changes are still allowed to be between `MAX_CHANGE_AGE`-old and `u32::MAX`-old in the interim between `check_tick` scans. While we prevent their age from overflowing, the test to detect changes still compares raw values. This makes failure ultimately unreliable, since when ancient changes stop being detected varies depending on when the next scan occurs.

## Open Question

Currently, systems and system states are incorrectly initialized with their `last_change_tick` set to `0`, which doesn't handle wraparound correctly.

For consistent behavior, they should either be initialized to the world's `last_change_tick` (and detect no changes) or to `MAX_CHANGE_AGE` behind the world's current `change_tick` (and detect everything as a change). I've currently gone with the latter since that was closer to the existing behavior.

## Follow-up Work

(Edited: entire section)

We haven't actually profiled how long a `check_ticks` scan takes on a "large" `World` , so we don't know if it's safe to increase their frequency. However, we are currently relying on play sessions not lasting long enough to trigger a scan and apps not having enough entities/archetypes for it to be "expensive" (our assumption). That isn't a real solution. (Either scanning never costs enough to impact frame times or we provide an option to use `u64` change ticks. Nobody will accept random hiccups.)

To further extend the lifetime of changes, we actually only need to increment the world tick if a system has `Fetch: !ReadOnlySystemParamFetch`. The behavior will be identical because all writes are sequenced, but I'm not sure how to implement that in a way that the compiler can optimize the branch out.

Also, since having no false positives depends on a `check_ticks` scan running at least every `2 * N - 1` ticks, a `last_check_tick` should also be stored in the `World` so that any lull in system execution (like a command flush) could trigger a scan if needed. To be completely robust, all the systems initialized on the world should be scanned, not just those in the current stage.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
## Objective

- ~~Make absurdly long-lived changes stay detectable for even longer (without leveling up to `u64`).~~
- Give all changes a consistent maximum lifespan.
- Improve code clarity.

## Solution

- ~~Increase the frequency of `check_tick` scans to increase the oldest reliably-detectable change.~~
(Deferred until we can benchmark the cost of a scan.)
- Ignore changes older than the maximum reliably-detectable age.
- General refactoring—name the constants, use them everywhere, and update the docs.
- Update test cases to check for the specified behavior.

## Related

This PR addresses (at least partially) the concerns raised in:

- bevyengine#3071
- bevyengine#3082 (and associated PR bevyengine#3084)

## Background

- bevyengine#1471

Given the minimum interval between `check_ticks` scans, `N`, the oldest reliably-detectable change is `u32::MAX - (2 * N - 1)` (or `MAX_CHANGE_AGE`). Reducing `N` from ~530 million (current value) to something like ~2 million would extend the lifetime of changes by a billion.

| minimum `check_ticks` interval | oldest reliably-detectable change  | usable % of `u32::MAX` |
| --- | --- | --- |
| `u32::MAX / 8`  (536,870,911) | `(u32::MAX / 4) * 3` | 75.0% |
| `2_000_000` | `u32::MAX - 3_999_999` | 99.9% |

Similarly, changes are still allowed to be between `MAX_CHANGE_AGE`-old and `u32::MAX`-old in the interim between `check_tick` scans. While we prevent their age from overflowing, the test to detect changes still compares raw values. This makes failure ultimately unreliable, since when ancient changes stop being detected varies depending on when the next scan occurs.

## Open Question

Currently, systems and system states are incorrectly initialized with their `last_change_tick` set to `0`, which doesn't handle wraparound correctly.

For consistent behavior, they should either be initialized to the world's `last_change_tick` (and detect no changes) or to `MAX_CHANGE_AGE` behind the world's current `change_tick` (and detect everything as a change). I've currently gone with the latter since that was closer to the existing behavior.

## Follow-up Work

(Edited: entire section)

We haven't actually profiled how long a `check_ticks` scan takes on a "large" `World` , so we don't know if it's safe to increase their frequency. However, we are currently relying on play sessions not lasting long enough to trigger a scan and apps not having enough entities/archetypes for it to be "expensive" (our assumption). That isn't a real solution. (Either scanning never costs enough to impact frame times or we provide an option to use `u64` change ticks. Nobody will accept random hiccups.)

To further extend the lifetime of changes, we actually only need to increment the world tick if a system has `Fetch: !ReadOnlySystemParamFetch`. The behavior will be identical because all writes are sequenced, but I'm not sure how to implement that in a way that the compiler can optimize the branch out.

Also, since having no false positives depends on a `check_ticks` scan running at least every `2 * N - 1` ticks, a `last_check_tick` should also be stored in the `World` so that any lull in system execution (like a command flush) could trigger a scan if needed. To be completely robust, all the systems initialized on the world should be scanned, not just those in the current stage.
@alice-i-cecile alice-i-cecile added the A-ECS Entities, components, systems, and events label Sep 8, 2024
@alice-i-cecile
Copy link
Member

I don't think this is particularly relevant any more. Closing.

@alice-i-cecile alice-i-cecile closed this as not planned Won't fix, can't repro, duplicate, stale Sep 8, 2024
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-Docs An addition or correction to our documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants