Refactor timers #11807

Swiftb0y · 2023-08-11T00:07:03Z

Initially wanted to investigate something performance-related but then got distracted by the questionable state of util/performancetimer.h and util/timer.h so I decided to clean it up a little.

src/util/battery/battery.cpp

Holzhaus · 2023-08-11T12:58:08Z

src/util/performancetimer.h

-// This is a fork of QPerformanceTimer just without the Q prefix
-// To fix interface changes issues in different QT versions
-// Added restart() function.


Have you checked if this class is still necessary? Maybe we can use plain QPerformanceTimer nowadays.

QPerformanceTimer doesn't exist anymore as far as I can tell. Closest thing would be QElapsedTimer. Didn't investigate whether that satisfies the same constraints. I will check.

QPerformanceTimer was forked from QElapsedTimer and later reintegrated:
qt/qt@8896651
I think we can adopt that commit to our code and drop our PerformanceTimer entirely.
I recommend that over the std::chrono::steady_clock solution because it is only weak defined by the C++ standard and may change by any update or different standard library.

Unfortunately the API differs a little bit because qt has no duration type. PerformanceTimer is also spread over more than 40 classes in our codebase. IMO we should keep PerformanceTimer as a facade over QElapsedTimer that makes use of mixxx::Duration (which we should also replace with std::chrono::duration` IMO but thats even harder than replacing PerformanceTimer verbatim).

Should we add a task to our Semantic Type Refactoring project https://github.com/mixxxdj/mixxx/projects/5 ?

Maybe. We should at least think about replacing the mixxx::Duration internals with std::chrono::duration to allow for gradual transition. But mixxx::Duration is already a semantic type, so I don't see a pressing need. The only major advantage std::chrono::duration has is its duration_cast as far as I can tell.

src/util/timer.h

daschuer · 2023-08-11T21:19:28Z

src/util/timer.h

+        if (m_state) {
+            m_state->cancelled = true;
+        }


The cancelled state can be replaced by m_state == std::nullopt;

Suggested change

if (m_state) {

m_state->cancelled = true;

}

m_state.reset();

Because the timers is no longer required and can be destructed.

I'm not super familiar with C++ destructors but I thought this would still happen automatically for RAII types even when using a user-provided constructor. Or is there a way to call a default-generated destructor within the user-provided destructor?

The cancelled state can be replaced by m_state == std::nullopt;

That implies the removal of the cancel method, but its only used in one location and thats before a return, so basically useless anyway.

daschuer · 2023-08-11T21:26:29Z

src/util/timer.h

-            initialize(QString(key), arg ? QString(arg) : QString(), compute);
-        }
+    ScopedTimer(QStringView key, int i, Stat::ComputeFlags compute = kDefaultComputeFlags)
+            : ScopedTimer(key, QString::number(i), compute) {


QString::number(i) constructs a QString without --developer.

fixed, now results in a duplicate getDeveloper() call, but I'm pretty sure that gets optimized away.

daschuer · 2023-08-11T22:20:56Z

src/util/performancetimer.h

-// This is a fork of QPerformanceTimer just without the Q prefix
-// To fix interface changes issues in different QT versions
-// Added restart() function.


QPerformanceTimer was forked from QElapsedTimer and later reintegrated:
qt/qt@8896651
I think we can adopt that commit to our code and drop our PerformanceTimer entirely.
I recommend that over the std::chrono::steady_clock solution because it is only weak defined by the C++ standard and may change by any update or different standard library.

…nterval

Swiftb0y · 2023-08-12T15:42:49Z

src/util/timer.h

    ~ScopedTimer() noexcept {
-        if (m_state) {
-            if (!m_state->cancelled) {
-                m_state->timer.elapsed(true);
-            }
+        if (m_maybeTimer) {
+            m_maybeTimer->elapsed(true);
        }
+        m_maybeTimer.reset();
    }


The reset triggers a -Wmaybe-uninitialized warning locally for me and I'm not sure why...

In destructor ‘QString::~QString()’, inlined from ‘Timer::~Timer()’ at /home/swiftb0y/Sourcerepositories/mixxx/src/util/timer.h:20:7, inlined from ‘constexpr void std::_Optional_payload_base<_Tp>::_M_destroy() [with _Tp = Timer]’ at /usr/include/c++/13/optional:287:35, inlined from ‘constexpr void std::_Optional_payload_base<_Tp>::_M_reset() [with _Tp = Timer]’ at /usr/include/c++/13/optional:318:14, inlined from ‘constexpr std::_Optional_payload<_Tp, false, _Copy, _Move>::~_Optional_payload() [with _Tp = Timer; bool _Copy = false; bool _Move = false]’ at /usr/include/c++/13/optional:439:65, inlined from ‘constexpr std::_Optional_base<Timer, false, false>::~_Optional_base()’ at /usr/include/c++/13/optional:510:12, inlined from ‘constexpr std::optional<Timer>::~optional()’ at /usr/include/c++/13/optional:705:11, inlined from ‘ScopedTimer::~ScopedTimer()’ at /home/swiftb0y/Sourcerepositories/mixxx/src/util/timer.h:71:5, inlined from ‘virtual void VinylControlXwax::analyzeSamples(CSAMPLE*, size_t)’ at /home/swiftb0y/Sourcerepositories/mixxx/src/vinylcontrol/vinylcontrolxwax.cpp:648:1: /usr/include/qt5/QtCore/qstring.h:1311:35: warning: ‘*(QString*)((char*)&t + offsetof(ScopedTimer, ScopedTimer::m_maybeTimer.std::optional<Timer>::<unnamed>.std::_Optional_base<Timer, false, false>::<unnamed>)).QString::d’ may be used uninitialized [-Wmaybe-uninitialized] 1311 | inline QString::~QString() { if (!d->ref.deref()) Data::deallocate(d); }

The Uninitialized QString seems to be Timer::m_key. The warning happens after the user destructor in the auto generated code.
It is uninitialized if m_maybeTimer is std::nullopt. But in that case the ~Timer() not be run.

src/sources/soundsourcemodplug.cpp

Swiftb0y · 2023-08-12T19:15:11Z

The QElapsedTimer::clockType() == PerformanceTimer debug assert fails on some platforms... How do we go about that? relax the restrictions and just insist its monotonic?

JoergAtGithub · 2023-08-12T19:23:45Z

According to the documentation QElapsedTimer::PerformanceCounter is a Windows implementation:
https://doc.qt.io/qt-5/qelapsedtimer.html#ClockType-enum

daschuer · 2023-08-12T22:51:02Z

src/util/timer.h

        }
+        m_maybeTimer.reset();


This will happen automatically.

Are you sure? Thats what I was asking in this comment: #11807 (comment)

Yes, the back trace with the warning #11807 (comment) proves it.
But there seems to be a related issue causing the warnig.

I cannot confirm that warning with valgrind, It seems to be a false positive.

daschuer · 2023-08-13T00:14:10Z

QElapsedTimer::clockType() seems to be not a nop() in some cases. Can we move the check to an extra function and call it only once?

In my case "QElapsedTimer::ClockType::MonotonicClock" is returned. QElapsedTimer::ClockType::PerformanceCounter" is nerver returned.

JoergAtGithub · 2023-08-13T05:31:06Z

Checking clock time make not much sense. The documentation says: "QElapsedTimer will always use the same clock type in a particular machine, so this value will not change". It just tells you which implementation is used on the particular system (result will differ on operating system).
What makes sense to check in an assertion is https://doc.qt.io/qt-5/qelapsedtimer.html#isMonotonic Also this needs to be checked only once on a particular machine.

daschuer · 2023-08-13T09:07:39Z

I agree. Since Qt uses the timer internternally for the same propose we, may rely on Qt that they will use the optimal timer.

Since system calls are involve, a subject to change with OS versions, we may even consider to check the monotonic nature it in release builds. So the check is in place for every "particular machine".

JoergAtGithub · 2023-08-13T10:10:43Z

On any modern desktop computer with any OS officially supported by Mixxx, you will get a monotonic clock.
I guess thiss is more relevant for the embedded and mobile platforms, which Qt also supports.

daschuer

Just a final nit pick and than LGTM

daschuer · 2023-08-20T07:00:30Z

src/util/performancetimer.h

+        // TODO: turn this into a static_assert and inline this method
+        // once Qt enables it.


This will probably never be a static assertion, because it depends on the OS executing Mixxx.

Suggested change

// TODO: turn this into a static_assert and inline this method

// once Qt enables it.

yes, but the OS / target is usually known at compile time anyways. That's why the stdlib can get a away with a constexpr static is_steady. https://en.cppreference.com/w/cpp/chrono/system_clock

Swiftb0y · 2023-08-20T11:58:30Z

I'm unsure whether the clock is monotonic in all cases. I experienced some test failures. https://github.com/Swiftb0y/mixxx/actions/runs/5913234283/job/16037422961

daschuer

LGTM, thank you.

Swiftb0y · 2023-08-20T12:28:13Z

you just merged dozens of fixup commits.... how do we proceed?

uklotzde · 2023-08-20T15:23:49Z

you just merged dozens of fixup commits.... how do we proceed?

@Swiftb0y Switch the repo to "Allow squash merging" exclusively for preventing this in the future? Avoids tons of low-value commits in the repo and ensures that git bisect works for every commit.

I have seen this strategy in a customer project recently and adopted it immediately after experiencing the benefits. Saves you from a lot of headaches.

Refactor timers (#11807) 2nd try

Swiftb0y added 11 commits August 11, 2023 02:06

feat(util): convert between mixxx::Duration and std::chrono::duration

c9cee81

refactor(util): implement PerformanceTimer using std::chrono

b3ca307

refactor(util): remove unused #include

6cf8d61

feat(util): make Stat::ComputeTypes a QFlag instead of plain int

c53a8bf

refactor(timer): Streamline QString use, ownership and temporaries

58ea29e

refactor(timer): make ScopedTimer invocations pass UTF-16 literals

e19e119

refactor(battery): use QTimer instead of GuiTickTimer for Battery Update

4e40862

refactor(timer): remove GuiTickTimer and SuspendableTimer (unused)

1af47b6

refactor(timer): optimize Timer running check

d3bf962

style(util): fix performancetimer indentation; add constexpr

fa31fb9

refactor(timer): cleanup ScopedTimer

9feecb5

github-actions bot added library ui build labels Aug 11, 2023

fixup! refactor(timer): cleanup ScopedTimer

991d876

Holzhaus reviewed Aug 11, 2023

View reviewed changes

daschuer reviewed Aug 11, 2023

View reviewed changes

src/util/timer.h Outdated Show resolved Hide resolved

daschuer reviewed Aug 11, 2023

View reviewed changes

daschuer requested changes Aug 11, 2023

View reviewed changes

Swiftb0y added 2 commits August 12, 2023 17:25

refactor(timer): replace chrono::steady_clock with QElapsedTimer

76c23f8

refactor(battery): make use of std::chrono::milliseconds for update i…

ee1f4fa

…nterval

Swiftb0y commented Aug 12, 2023

View reviewed changes

daschuer reviewed Aug 12, 2023

View reviewed changes

src/sources/soundsourcemodplug.cpp Outdated Show resolved Hide resolved

fixup! refactor(timer): cleanup ScopedTimer

482a425

Swiftb0y force-pushed the refactor/timers branch from 93a5644 to 482a425 Compare August 12, 2023 18:10

fixup! fixup! refactor(timer): cleanup ScopedTimer

7528cb4

daschuer reviewed Aug 12, 2023

View reviewed changes

JoergAtGithub requested a review from Holzhaus August 13, 2023 05:23

Swiftb0y added 3 commits August 19, 2023 21:49

fixup! refactor(timer): replace chrono::steady_clock with QElapsedTimer

0b7a2d7

fixup! fixup! refactor(timer): cleanup ScopedTimer

f2c090b

Merge remote-tracking branch 'upstream/main' into refactor/timers

9382ff5

daschuer requested changes Aug 20, 2023

View reviewed changes

fixup! refactor(timer): replace chrono::steady_clock with QElapsedTimer

cd86d9f

github-actions bot added the code quality label Aug 20, 2023

fixup! refactor(timer): replace chrono::steady_clock with QElapsedTimer

e39df37

daschuer approved these changes Aug 20, 2023

View reviewed changes

daschuer merged commit 1c4096e into mixxxdj:main Aug 20, 2023
8 of 10 checks passed

Swiftb0y mentioned this pull request Aug 21, 2023

Refactor timers (#11807) 2nd try #11850

Merged

JoergAtGithub added a commit that referenced this pull request Sep 6, 2023

Merge pull request #11850 from Swiftb0y/refactor/timers

8a16387

Refactor timers (#11807) 2nd try

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor timers #11807

Refactor timers #11807

Swiftb0y commented Aug 11, 2023

Holzhaus Aug 11, 2023

Swiftb0y Aug 11, 2023

daschuer Aug 11, 2023

Swiftb0y Aug 12, 2023

JoergAtGithub Aug 12, 2023

Swiftb0y Aug 12, 2023

daschuer Aug 11, 2023

Swiftb0y Aug 12, 2023

Swiftb0y Aug 12, 2023

daschuer Aug 11, 2023

Swiftb0y Aug 12, 2023

daschuer Aug 11, 2023

Swiftb0y Aug 12, 2023

daschuer Aug 12, 2023

Swiftb0y commented Aug 12, 2023

JoergAtGithub commented Aug 12, 2023

daschuer Aug 12, 2023

Swiftb0y Aug 12, 2023

daschuer Aug 12, 2023 •

edited

daschuer Aug 13, 2023

daschuer commented Aug 13, 2023

JoergAtGithub commented Aug 13, 2023

daschuer commented Aug 13, 2023

JoergAtGithub commented Aug 13, 2023

daschuer left a comment

daschuer Aug 20, 2023

Swiftb0y Aug 20, 2023

Swiftb0y commented Aug 20, 2023

daschuer left a comment

Swiftb0y commented Aug 20, 2023

uklotzde commented Aug 20, 2023

		// TODO: turn this into a static_assert and inline this method
		// once Qt enables it.

Refactor timers #11807

Refactor timers #11807

Conversation

Swiftb0y commented Aug 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Aug 12, 2023

JoergAtGithub commented Aug 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer Aug 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Aug 13, 2023

JoergAtGithub commented Aug 13, 2023

daschuer commented Aug 13, 2023

JoergAtGithub commented Aug 13, 2023

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y commented Aug 20, 2023

daschuer left a comment

Choose a reason for hiding this comment

Swiftb0y commented Aug 20, 2023

uklotzde commented Aug 20, 2023

daschuer Aug 12, 2023 •

edited