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 #15683

Open
bas-ie opened this issue Oct 6, 2024 · 2 comments
Open

Use u64 for change ticks #15683

bas-ie opened this issue Oct 6, 2024 · 2 comments
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! S-Blocked This cannot move forward until something else changes S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help

Comments

@bas-ie
Copy link
Contributor

bas-ie commented Oct 6, 2024

Note

Tracking issue for closed 2022 PR #6327 (alternate approach: #6651)
Original author: zakarumych

What problem does this solve or what need does it fill?

Avoids the complexity (and runtime perf overhead) of our current looping u32 implementation, at the cost of higher memory usage.

Implementation blocked on configurable change detection.

What solution would you like?

Replace u32 with u64 for tick values. Maintenance code to guard against overflow should be removed.

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

What alternative(s) have you considered?

See #6651 for context.

@bas-ie bas-ie added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Feature A new feature, making something new possible S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! S-Blocked This cannot move forward until something else changes S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help labels Oct 6, 2024
@alice-i-cecile
Copy link
Member

I've updated the motivation a bit :) Thanks for extracting this!

@alice-i-cecile alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times and removed C-Code-Quality A section of code that is hard to understand or change labels Oct 6, 2024
@BD103
Copy link
Member

BD103 commented Oct 7, 2024

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

It is specifically PowerPC and MIPS that do not support AtomicU64, since I tried using it in Cargo.

It looks like MIPS is being phased out in favor of RISC-V. PowerPC also looks like it is dying, though it is notably used by the GameCube, Wii, and Wii U. If we want to support these platforms (which I think would be awesome), we definitely want some workaround.

@BenjaminBrienen BenjaminBrienen added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label Oct 30, 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-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! S-Blocked This cannot move forward until something else changes S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help
Projects
None yet
Development

No branches or pull requests

4 participants