Use 64 bit for world and system tick index #6651
Conversation
alice-i-cecile
added
A-ECS
There was a problem hiding this comment.
Only a cursory scan. Have not done a full review of the updated logic just yet.
You do not need #[inline(always)] for most of these functions at this size. They're almost always guaranteed to inline even in debug mode.
Again, like #6327, this needs benchmarking to show the changes in performance here. This also has sizable overlap with #6547, which does something similar on the storage side. There's inevitably going to be merge conflicts between the two.
| system_meta: &SystemMeta, | ||
| world: &'w World, | ||
| change_tick: u32, | ||
| change_tick: Tick, |
There was a problem hiding this comment.
| change_tick: Tick, | |
| change_tick: #path::change_detection::Tick, |
| /// than `SMALL_TICK_AGE_THRESHOLD` may return false positive. | ||
| /// Which is acceptable tradeoff given that too old targets should | ||
| /// not occur outside some edge cases. | ||
| /// |
| } | ||
|
|
||
| impl Tick { | ||
| #[inline(always)] |
There was a problem hiding this comment.
| #[inline(always)] |
| pub struct Tick(pub NonZeroU64); | ||
|
|
||
| impl Display for Tick { | ||
| #[inline(always)] |
There was a problem hiding this comment.
| #[inline(always)] |
| } | ||
|
|
||
| impl SmallTick { | ||
| #[inline(always)] |
There was a problem hiding this comment.
| #[inline(always)] |
| /// not occur outside some edge cases. | ||
| /// | ||
| #[repr(transparent)] | ||
| #[derive(Clone, Copy, Debug)] |
There was a problem hiding this comment.
| #[derive(Clone, Copy, Debug)] | |
| #[derive(Clone, Copy, Debug, Default)] |
| impl Default for SmallTick { | ||
| #[inline(always)] | ||
| fn default() -> Self { | ||
| SmallTick::new() | ||
| } | ||
| } |
There was a problem hiding this comment.
| impl Default for SmallTick { | |
| #[inline(always)] | |
| fn default() -> Self { | |
| SmallTick::new() | |
| } | |
| } |
| impl Default for Tick { | ||
| #[inline(always)] | ||
| fn default() -> Self { | ||
| Tick::new() | ||
| } | ||
| } |
There was a problem hiding this comment.
| impl Default for Tick { | |
| #[inline(always)] | |
| fn default() -> Self { | |
| Tick::new() | |
| } | |
| } |
| } | ||
| } | ||
|
|
||
| // #[allow(clippy::useless_conversion)] |
There was a problem hiding this comment.
| // #[allow(clippy::useless_conversion)] |
| // Safety: | ||
| // `TICK_BASE_VALUE` is nonzero |
There was a problem hiding this comment.
| // Safety: | |
| // `TICK_BASE_VALUE` is nonzero | |
| // SAFETY: `TICK_BASE_VALUE` is nonzero |
james7132
added
the
S-Needs-Benchmarking
|
#6547 has been merged. Can you rebase? I'll run a set of microbenchmarks for comparison. |
zakarumych
force-pushed
the
update-ticks
branch
from
b92d7e0 to
fccdd3b
Compare
ItsDoot
added
the
S-Adopt-Me
|
Backlog cleanup: closing this and alternate approach #6327 due to inactivity and no clear consensus. |
5 participants
Objective
Changelog
TickandSmallTick.Ticknow containsNonZeroU64and never overflow in any reasonable time.SmallTickare used to store tick index when component addition and last modification. It containsu32with lower 32 bits ofTicks 64 bit value.Worldnow usesTickCounterthat implements tick incrementing and necessary bookkeeping.Migration Guide
Systemimplementations have to switch to strongly typedTickvalues and remove few methods.Pick one
This is alternative to #6327 where components get their ticks grown to 64 bit as well.
And #6327 doesn't introduce strongly typed ticks. Can be implemented though.