mixxx icon indicating copy to clipboard operation
mixxx copied to clipboard

Duration: Use double instead of quint64 to store nanoseconds internally

Open Holzhaus opened this issue 2 years ago • 8 comments

As discussed here: https://github.com/mixxxdj/mixxx/pull/4138#issuecomment-886057249

NOTE: Test fail locally and I have no idea why (but also didn't spend too much time on debugging yet). Maybe CI output will show what the issue is.

Holzhaus avatar Jul 26 '21 16:07 Holzhaus

hmm, I think it'd be a shame to lose fast, precise equality checking and fast integer math calculations for durations

rryan avatar Jul 26 '21 18:07 rryan

hmm, I think it'd be a shame to lose fast, precise equality checking and fast integer math calculations for durations

How much faster is a 64bit int compare to a double compare? I don't think we loose many places with integer math.

daschuer avatar Jul 26 '21 23:07 daschuer

Pull Request Test Coverage Report for Build 1070724499

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 28.611%

Totals Coverage Status
Change from base Build 1070321390: 0.0%
Covered Lines: 20083
Relevant Lines: 70193

💛 - Coveralls

coveralls avatar Jul 27 '21 10:07 coveralls

hmm, I think it'd be a shame to lose fast, precise equality checking and fast integer math calculations for durations

In the long term we should better switch to std::chrono::duration. Probably with different representations for different purposes and use cases. Typedefs in Mixxx should work fine as they will result in distinct types if either the representation type or the period differs.

uklotzde avatar Jul 27 '21 11:07 uklotzde

I agree that using double for performance timers is not optimal: https://github.com/Holzhaus/mixxx/pull/11

A TODO should suffice for the moment.

uklotzde avatar Jul 27 '21 22:07 uklotzde

Hmm, if I introduce the beatLength() a lot of test fail because they depend on the miniscule rounding error differences. I'm currently questioning if it's worth it.

Holzhaus avatar Jul 27 '21 22:07 Holzhaus

What is the state of this PR? Is it still in a mergeable state? Should we do a review?

daschuer avatar Oct 21 '21 21:10 daschuer

This PR is marked as stale because it has been open 90 days with no activity.

github-actions[bot] avatar Jan 25 '22 00:01 github-actions[bot]