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

Change tracking value type,from u32 to u64. #214

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

80ROkWOC4j
Copy link

@80ROkWOC4j 80ROkWOC4j commented Jan 6, 2025

This PR is submitted to mitigate the issues in issue #190.
My project is suffering from a bug that is believed to be caused by this.
In most cases, changing from u32 to u64 will not be a significant drawback, and conversely, there will be few cases where it is absolutely necessary, so I will defer to the maintainer's decision.

cargo test and cargo make test done.
ps. is cargo make test-all still valid?

@leudz
Copy link
Owner

leudz commented Jan 8, 2025

#190 was fixed somewhere between v0.6 and v0.7.

In most cases, changing from u32 to u64 will not be a significant drawback

My main concern with this change is memory. Insertion and modification tracking each use a TrackingTimestamp per component.

pub(crate) insertion_data: Vec<TrackingTimestamp>,
pub(crate) modification_data: Vec<TrackingTimestamp>,
pub(crate) deletion_data: Vec<(EntityId, TrackingTimestamp, T)>,
pub(crate) removal_data: Vec<(EntityId, TrackingTimestamp)>,

I don't think increasing the size is the solution. A u32 should hold something like 100 hours before any false positive is possible. And false positives shouldn't really happen with this gap.
I'd rather we talk about the issue you're having. How long your program runs before you see issues? How many systems have run?

ps. is cargo make test-all still valid?

No, I switched to just some time ago. I forgot to update CONTRIBUTING.md, thanks for bringing up the old cargo make command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants