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

[Merged by Bors] - Make change lifespan deterministic and update docs #3956

Closed
wants to merge 23 commits into from

Conversation

maniwani
Copy link
Contributor

@maniwani maniwani commented Feb 15, 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:

Background

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 A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels Feb 15, 2022
@alice-i-cecile
Copy link
Member

The value chosen in this PR does reduce the capacity of a stage from ~530 million systems to ~2 million

2 million systems should be enough for anyone ;) I think this is a good tradeoff.

Since "no false positives" depends on a check_ticks scan running at least every 2x the min. interval ticks, last_tick_check should be stored in World instead of SystemStage

Agreed. This should be done as part of Stageless.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Much easier to follow now! I've left a few thoughts and requests.

crates/bevy_ecs/src/system/system.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/function_system.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/change_detection.rs Outdated Show resolved Hide resolved
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This looks good now. I'm fine to merge this, and leave further optimizations for the future.

Copy link
Contributor

@Davier Davier left a comment

Choose a reason for hiding this comment

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

Increase the frequency of check_tick scans to increase the oldest reliably-detectable change.

I don´t have a strong opinion on that, but I have something to add to discuss the trade-off. The reason why we chose a large CHECK_TICK_THRESHOLD is that the scan involves iterating every component, and may result in a noticeable stutter. It´s better if it never happens. At the current number, an app with 1000 systems run per frame at 144fps will scan once every hour. I´m not very comfortable reducing that number.
Not increasing the count for systems that are read only would improve the situation, and making change detection out-out per-component would give an escape hatch for those that need it.
I suppose that increasing the range of detection is valuable to you, could you comment on what you are doing?

Ignore changes older than the maximum reliably-detectable age.

See my in-line comment on that.

Add appropriately named constants in an appropriate place and use them everywhere.

The refactoring and comments are nice!

crates/bevy_ecs/src/component.rs Outdated Show resolved Hide resolved
@maniwani
Copy link
Contributor Author

The reason why we chose a large CHECK_TICK_THRESHOLD is that the scan involves iterating every component, and may result in a noticeable stutter. It´s better if it never happens.

This is good to know. I'm glad I tried to change the threshold then because I did not see that in the original issue (maybe I just missed it tho).

Between saving space and avoiding a potential stutter (we should try to test this), which is more important? If we really never want to run a scan, we should bump to u64.

I do have a reason to prefer u32. Change tick values probably need to be replicated by networking, and moving to u64 would double the amount of bytes that need to be copied.

I suppose that increasing the range of detection is valuable to you, could you comment on what you are doing?

I mostly made this PR because I'm working on implementing stageless, which removes SystemStage, so I had to understand how this check_tick stuff works to know where to put it back in.

I remembered #3071, and u32::MAX / 8 seemed like a very sub-optimal value, so I changed it. I wasn't aware there was more thought behind it.

@alice-i-cecile
Copy link
Member

IMO saving space is more important for now. And reducing stutter is more important than increasing the max lifespan.

We can consider bumping up to a u64 again if and when this is opt in.

@maniwani
Copy link
Contributor Author

1000 ticks per frame at 144 frames per second will scan once every hour

So I just pushed a change to raising the check tick threshold to this value, but I'm immediately thinking I should reverse it. Choosing this huge number is a pre-emptive solution to a problem that AFAIK we aren't sure exists. If performing a scan causes frame stuttering, any frequency is probably unacceptable, even once per hour. Players won't ignore that.

@maniwani maniwani changed the title Extend detectable lifespan of changes Make change lifespan deterministic and update docs Mar 3, 2022
Copy link
Contributor Author

@maniwani maniwani left a comment

Choose a reason for hiding this comment

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

Wanted to get some confirmation on this change specifically.

(Edit: Slight mistake. I think we should initialize new systems and system states so they see all data as changed. To account for wraparound, this means initializing their last_change_tick to be MAX_CHANGE_AGE behind the current world tick, not 0.)

Comment on lines 142 to 145
impl<Param: SystemParam> SystemState<Param> {
pub fn new(world: &mut World) -> Self {
let mut meta = SystemMeta::new::<Param>();
meta.last_change_tick = world.last_change_tick;
let param_state = <Param::Fetch as SystemParamState>::init(world, &mut meta);
Copy link
Contributor Author

@maniwani maniwani Mar 3, 2022

Choose a reason for hiding this comment

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

Having last_change_tick initialized to 0 was probably not correct. The counter may have just wrapped around, and if the world tick was at, say, 10 (which could really be u32::MAX + 11), ComponentTicks::is_changed would ignore any changes older than 10 ticks.

During initialization, last_change_tick is used as-is to init all the system params. It doesn't get updated until after the data is fetched by the first get.

Copy link
Member

Choose a reason for hiding this comment

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

This change makes sense to me.

Copy link
Contributor Author

@maniwani maniwani Mar 3, 2022

Choose a reason for hiding this comment

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

I think I still have it wrong. Like this, system states initialized "late" wouldn't see any changes. I think to be consistent, the last_change_tick should be initialized to world.change_tick().wrapping_sub(MAX_CHANGE_AGE) to detect everything as a change (everything that can be detected anyway).

Copy link
Contributor

Choose a reason for hiding this comment

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

Having last_change_tick initialized to 0 was probably not correct.

Agreed

Like this, system states initialized "late" wouldn't see any changes.

I would personally expect them to only detect changes that occurred after they were initialized. Both behaviours have their merits, I think the question should be made very visible in the PR so that reviewers don't miss it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll add it to the description.

crates/bevy_ecs/src/system/function_system.rs Outdated Show resolved Hide resolved
@maniwani
Copy link
Contributor Author

maniwani commented Mar 3, 2022

OK, I think that's everything.

Copy link
Contributor

@Davier Davier left a comment

Choose a reason for hiding this comment

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

All the refactoring looks good, and I'm fine with the change in system initialization

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 7, 2022
@mockersf
Copy link
Member

mockersf commented Mar 7, 2022

I don't know why CI is not running on this PR... that really bothers me

@mockersf
Copy link
Member

mockersf commented Mar 7, 2022

bors try

bors bot added a commit that referenced this pull request Mar 7, 2022
@bors
Copy link
Contributor

bors bot commented Mar 7, 2022

try

Build failed:

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 9, 2022

@maniwani can you please mark that this fixes #3082 in the PR description so we don't forget to close that out?

@maniwani
Copy link
Contributor Author

maniwani commented Mar 9, 2022

Can you please mark that this fixes #3082 in the PR description so we don't forget to close that out?

done

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

Just a couple of docs changes. You only need to use ChangeTrackers if you need to know multiple things about added/changed state in your system’s logic. If you only need one then you can use Added or Changed in your query (not the query’s filters) to get a book that tells you whether or not that happened for the component since the last time the system was executed.

/// If instead behavior is meant to change on whether the component changed or not
/// [`ChangeTrackers`](crate::query::ChangeTrackers) may be used.
/// To retain all results without filtering but still check whether they were added after the
/// system last ran, use [`ChangeTrackers<T>`](crate::query::ChangeTrackers).
Copy link
Contributor

Choose a reason for hiding this comment

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

Or use Added<T> in your query instead of in its filters and you will get a bool indicating whether that component was added or not since the last execution of the system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thanks for pointing that out. Can you share a token example to add to the docs?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I having trouble coming up with self-contained and not completely artificial applications.

Copy link
Contributor

Choose a reason for hiding this comment

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

#4180 The performance gains there are due to avoiding the additional lookup in the filtered query. If you just get the changed state directly in the query the same information is right there.

/// If instead behavior is meant to change on whether the component changed or not
/// [`ChangeTrackers`](crate::query::ChangeTrackers) may be used.
/// To retain all results without filtering but still check whether they were changed after the
/// system last ran, use [`ChangeTrackers<T>`](crate::query::ChangeTrackers).
Copy link
Contributor

Choose a reason for hiding this comment

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

Or use Changed<T> in your query instead of the filters and you will get a bool indicating whether the component was added or changed since the last time this system was executed.

@maniwani maniwani mentioned this pull request Apr 1, 2022
7 tasks
@alice-i-cecile alice-i-cecile added this to the Bevy 0.8 milestone Apr 25, 2022
maniwani added 16 commits May 9, 2022 06:35
Since `check_tick` scans are infrequent, you just might see the warnings for a bunch of different systems (might be worth to batch and print a single message). It's true tho that we'll repeat warnings on the next scan if nothing changes.
people have asked before if systems detect their own changes, hopefully this helps
…hing

new system (state) instances should detect everything as a change
why is dereferenced so hard to spell
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request 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
Copy link
Contributor

bors bot commented May 9, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request 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 bors bot changed the title Make change lifespan deterministic and update docs [Merged by Bors] - Make change lifespan deterministic and update docs May 9, 2022
@bors bors bot closed this May 9, 2022
robtfm pushed a commit to robtfm/bevy that referenced this pull request 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 pull request 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.
@maniwani maniwani deleted the change-is-forever branch July 28, 2022 22:59
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request 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
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants