Duration: Use double instead of quint64 to store nanoseconds internally

[Holzhaus]

As discussed here: #4138 (comment)

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.

[uklotzde]

return static_cast<quint64>(toDoubleSeconds())?

The comment "(rounded-down)" could be misleading unless we prevent negative durations.

[daschuer]

It looks like this is dead code anyway. Can we remove it?

[Holzhaus]

It's true that this method is only used by the class itself and by tests:

$ grep -HrnF toIntegerSeconds ../src
../src/test/duration_test.cpp:13:    EXPECT_EQ(0, d.toIntegerSeconds());
../src/test/duration_test.cpp:19:    EXPECT_EQ(1, d.toIntegerSeconds());
../src/test/duration_test.cpp:27:    EXPECT_EQ(0, d.toIntegerSeconds());
../src/test/duration_test.cpp:33:    EXPECT_EQ(1e3, d.toIntegerSeconds());
../src/test/duration_test.cpp:41:    EXPECT_EQ(0, d.toIntegerSeconds());
../src/test/duration_test.cpp:47:    EXPECT_EQ(1e6, d.toIntegerSeconds());
../src/test/duration_test.cpp:55:    EXPECT_EQ(255, d.toIntegerSeconds());
../src/test/duration_test.cpp:61:    EXPECT_EQ(1e9, d.toIntegerSeconds());
../src/test/duration_test.cpp:69:    EXPECT_EQ(5, d.toIntegerSeconds());
../src/test/duration_test.cpp:70:    EXPECT_EQ(2, d2.toIntegerSeconds());
../src/test/duration_test.cpp:71:    EXPECT_EQ(7, d3.toIntegerSeconds());
../src/test/duration_test.cpp:79:    EXPECT_EQ(7, d.toIntegerSeconds());
../src/test/duration_test.cpp:80:    EXPECT_EQ(2, d2.toIntegerSeconds());
../src/test/duration_test.cpp:88:    EXPECT_EQ(5, d.toIntegerSeconds());
../src/test/duration_test.cpp:89:    EXPECT_EQ(2, d2.toIntegerSeconds());
../src/test/duration_test.cpp:90:    EXPECT_EQ(3, d3.toIntegerSeconds());
../src/test/duration_test.cpp:98:    EXPECT_EQ(-5, d.toIntegerSeconds());
../src/test/duration_test.cpp:99:    EXPECT_EQ(10, d2.toIntegerSeconds());
../src/util/duration.h:25:    constexpr qint64 toIntegerSeconds() const {
../src/util/duration.h:115:            return debug << dd.toIntegerSeconds() << "s";
../src/util/duration.h:255:        return QString("%1 s").arg(toIntegerSeconds());

However, the other toIntegerFoo() methods are used elsewhere, and it would be weird that such a method is only missing for a single unit, but provided for others.

[rryan]

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

[daschuer]

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]

seconds should be casted to a double first.
Do we still need a template? I think If we make the seconds a double we can even remove the cast.

[daschuer]

kNanosPerSecond should be also a double to avoid loss of precision warnings.

[Holzhaus]

I made kNanosPerSecond a double. I'll check if removing the template breaks anything.

[coveralls]

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
Change from base Build 1070321390:	0.0%
Covered Lines:	20083
Relevant Lines:	70193

---

💛 - Coveralls

[uklotzde]

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]

I agree that using double for performance timers is not optimal: Holzhaus#11

A TODO should suffice for the moment.

[Holzhaus]

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.

[daschuer]

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

[github-actions]

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