Improve change detection warning message #3071
Labels
A-ECS
Entities, components, systems, and events
C-Docs
An addition or correction to our documentation
C-Usability
A targeted quality-of-life change that makes Bevy easier to use
Comments
alice-i-cecile
added
C-Docs
|
Hmm yeah we didn't include "many thousands of frames per second" in our calculations + discussions, which does seem like a bit of a miss. Default frame limiting would go a long way in preventing this. Making change detection opt-in doesn't solve the problem, it just brushes it under the rug. If we're going to provide a feature, it should probably work as expected! Moving to u64 would definitely help though. Worth doing some benchmarks / weighing the cost of the change. |
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.
|
This is basically fixed. |
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
How can Bevy's documentation be improved?
This error message was actually hit by a user after they left a game running overnight.
This caused a large number of warnings, and the user was left confused.
We should:
debugonly.Changed,Addedand the.is_changedand.is_addedmethods.If this is a regular occurence, we should probably swap change-detection to be opt-in / opt-out and move to a
u64.The text was updated successfully, but these errors were encountered: