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

Use u64 for change ticks #6327

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

zakarumych
Copy link

Objective

Allows detecting arbitrary old changes
Simplifies a lot of code along the way
Would play nice with query that detects changes since arbitrary tick that I'll implement next.

Solution

Replace u32 with u64 for tick values.
Maintenance code to guard against overflow is simply removed.

Downside - requires AtomicU64.
A workaround should be implemented for when AtomicU64 is not available on target platform.


Changelog

Changed

Type of values that represent ticks are changed from u32 to u64.
World's change_tick is AtomicU64 now.

Migration Guide

Some trait methods became obsolete and were removed.
Many users would need to change nothing as said traits are rarely used directly.

Allows detecting arbitrary old changes
Simplifies a lot of code along the way
@james7132
Copy link
Member

I would very much like #4972 merged before we attempt to merge this. This doubles the memory footprint of change detection, which will double the cache miss rate on all change detection filters and any query with mutative access. This potential regression should be weighed against the periodic cost of running check_change_ticks, as well as further optimizations like splitting ComponentTicks.

@zakarumych
Copy link
Author

zakarumych commented Oct 21, 2022

Periodic cost of check_change_ticks is not the problem.
The problem is inability to detect old changes which will get in the way when using changed_since(tick) query if tick is old.
Memory footprint is a problem. But I don't thing it is severe.
Change detection filters should be optimized to not even read change ticks of every component.

I can port optimization that I've made in edict ECS: latest change tick among a fixed slice of components in column is stored in array and latest change tick of whole column as well, so filters can skip columns and medium sized chunks of components and only inspect individual change ticks when there may be one matching.

@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 C-Usability A simple quality-of-life change that makes Bevy easier to use S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help labels Oct 21, 2022
@alice-i-cecile
Copy link
Member

Agreed with James on benchmarking. I'd also like the ability to opt-out of change detection as well, with ZSTs opted out by default. Marker components are very common and powerful, and I'd really like to avoid incurring performance costs to using them.

While working on a production game, memory cost is one of my larger performance concerns (much higher than runtime CPU speed), and I think we can and should do better.

I can port optimization that I've made in edict ECS: latest change tick among a fixed slice of components in column is stored in array and latest change tick of whole column as well, so filters can skip columns and medium sized chunks of components and only inspect individual change ticks when there may be one matching.

This would be very appreciated; I think this is a great direction, regardless of what we decide for the question in this PR.

@zakarumych
Copy link
Author

Unfortunately I can't see how to implement changed_since(tick) query when ticks can wrap around. Argument tick can be from previous lap and incorrectly assumed "later" than last_change_tick.

Opt-out change tracking would help with memory cost.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Oct 21, 2022

Unfortunately I can't see how to implement changed_since(tick) query when ticks can wrap around. Argument tick can be from previous lap and incorrectly assumed "later" than last_change_tick.

We should make a couple assumptions here:

  1. The provided change_tick is always earlier than or equal to the current change_tick.
  2. When dealing with wraparound, we can use the same logic as currently exists.

This method should just call self.ticks.component_ticks.is_changed, but passing in a manual last_changed_tick value :)

IMO we should be making that first assumption (that last_changed_tick is early than current_change_tick) throughout.

@zakarumych
Copy link
Author

zakarumych commented Oct 21, 2022

Imagine following situation.

component.last_change_tick = 42; // after one wrap around
world.current_tick = 100; // after one wrap around
query.last_change_tick = 43 // before wrap around

What should happen?
It should see component as changed, but there's now info that value of query.last_change_tick is from previous wrap around to distinguish from current 43.

Current logic uses method check_change_tick to modify system.last_change_tick - bumps it to world.change_tick.wrapping_sub(MAX_CHANGE_AGE).

In case of changed_since(tick) query the tick value is under user's control.

Possibly we can make only world.change_tick: u64 and keep it as u32 for components.
It'll probably work with the same downside as current code does - loosing about old changes.

@maniwani
Copy link
Contributor

maniwani commented Oct 21, 2022

This potential regression should be weighed against the periodic cost of running check_change_ticks, as well as further optimizations like splitting ComponentTicks.

(Edit) I support this change, but I agree with @james7132.

I don't think change ticks being u32 blocks your proposed query. It would just be limited to MAX_CHANGE_AGE.

Using u64 is simpler and means changes would never expire, but it doubles the memory use. What kinds of apps do you expect to run long enough to have such old changes?

#3956 goes over the reasoning behind the current value of MAX_CHANGE_AGE. I wanted to make it much larger (~99% of u32 range), but that would increase the check_change_tick scan frequency and we still haven't benchmarked it.

Was also trying to be mindful of their footprint in case we have applications that want to send them in packets.

@zakarumych
Copy link
Author

What kinds of apps do you expect to run long enough to have such old changes?

u32 may overflow in days. A game server can run that long and more.

I don't think change ticks being u32 blocks your proposed query. It would just be limited to MAX_CHANGE_AGE.

That would require changing at least World's current_tick type to u64

@maniwani
Copy link
Contributor

maniwani commented Oct 23, 2022

A game server can run that long and more.

I think I asked the wrong question. Can you give a usage example where an app would be querying for an extremely old change?

I'm looking for scenarios that would motivate doing this sooner, but atm it seems very niche. Normally, game sessions don't persist for days and systems don't run that infrequently.

Edit:

That would require changing at least World's current_tick type to u64.

Yeah, that seems like a fair compromise, though I would add this u64 while keeping the u32 for the existing change detection so we don't have to do % everywhere. I forgot we could just read the lower bits.

@zakarumych
Copy link
Author

Systems do not run this infrequently. The scenario I have in my head concerns changed_since(tick) queries. Where tick can be pretty old. For example a system may query for all changes that occurred since some in-game event. Not yielding really old changes may cause errors in game logic. It would require to add workarounds, like yielding everything ignoring change ticks if tick is too old.
It's true that only long enough game sessions can be affected. Most games nowadays are short play-session based. But not all of them.

@james7132
Copy link
Member

Yes but, even with thousands of systems, over the course of days you wouldn't easily run into this. Detecting a change this old is borderline cronjob (i.e. weekly maintenance) level operations, for which this seems like a very poor approach to handling.

@james7132
Copy link
Member

Yes but, even with thousands of systems, over the course of days you wouldn't easily run into this. Detecting a change this old is borderline cronjob (i.e. weekly maintenance) level operations, for which this seems like a very poor approach to handling.

EDIT: May have miscalculated. A 2,000 system game running at 60 TPS runs over the max change age every 7 hours. I still think this is a rather atypical use case, but I can see this happening regularly on a MMO or any other persistent game server.

@zakarumych
Copy link
Author

this happening regularly on a MMO or any other persistent game server.

And there are lots of simpler games with persistent game server. And single player games can be running for days too.

I agree that benchmarks should be used to see if this change hurts performance too much.

Before that opt-out from changing tracking for components should be implemented.
And at this point component may even choose to track changes using u32 that may wrap-around with some downsides or u64 without them. Or u16, why not?

Together with u64 in World and in changed_since(tick) API to not confuse too old tick value (user owned, can't bump automatically as in systems) with newer one this can actually work pretty well.

@james7132
Copy link
Member

If we merge #6547, this change will likely be perf neutral with the current status quo other than the 2x memory usage for change detection. I'll run microbenchmarks for this PR if/when that one is merged.

@zakarumych
Copy link
Author

I've prepared another PR with the same idea but that keeps using u32 for tick values of components.
Going to submit it and close this one.

Not sure what would be better. To add changed_since(tick) feature and opt-out from change tracking into the same PR or make them separate?

@james7132
Copy link
Member

Separate would be easier. The larger (and more controversial) it becomes, the harder it becomes to review and convince others that it's the right direction.

@maniwani
Copy link
Contributor

Going to submit it and close this one.

Please leave this open. If #6547 gets merged, I think this one would likely be accepted. 2x memory use does seem like it would affect wasm32, but I think the utility of changed_since(tick) and the simplified logic is worth it.

@zakarumych
Copy link
Author

zakarumych commented Nov 16, 2022

I've created #6651 anyway. Feel free to compare and pick one.
Both allow simple changed_since(tick) implementation.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Feb 12, 2023
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 C-Usability A simple quality-of-life change that makes Bevy easier to use S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants