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

Fix loss of precision in AnimClock and improve performance #24

Merged
merged 1 commit into from
May 17, 2022

Conversation

SUPERCILEX
Copy link
Contributor

Old:

1s 320ms
Left: 140292640
Right: 140292630

New:

759ms
Success

@SUPERCILEX
Copy link
Contributor Author

Oh, for whatever reason now I'm seeing ~1s time for the old version. The new one is still faster, but not by as much I guess.

@djeedai
Copy link
Owner

djeedai commented May 16, 2022

If you want stable results you need to bench it. This test is not warming the codepath before measuring, so will have variations per run. An ideal perf test runs the code first a few times so that all caches are hot and the CPU frequency scaling if any is already in power/perf mode (and ideally disables low-power and frequency scaling, but that's not always possible). That's probably good enough though given the large difference before/after.

@SUPERCILEX
Copy link
Contributor Author

Yeah, I'm being very lazy. I mainly just wanted to check that using u128s wasn't slower than the old stuff.

@djeedai
Copy link
Owner

djeedai commented May 17, 2022

I'm going to merge as is for now, but I think the removal of times_completed will have to be reverted for #16 to work. Anyway, we'll see.

@djeedai djeedai merged commit ab1e792 into djeedai:main May 17, 2022
@SUPERCILEX SUPERCILEX deleted the precision branch May 17, 2022 21:15
@SUPERCILEX
Copy link
Contributor Author

Sounds good! I don't have time this week, but I'll start working on your ideas for #19 (unless you get to it first 😄) which will let me see what's still applicable in #21.

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