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

Refactor/modernize shrink scopedtimer #13236

Closed
42 changes: 42 additions & 0 deletions src/util/qstringformat.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#pragma once

#include <QString>
#include <type_traits>
#include <utility>

namespace {

// taken from Qt
template<typename T>
static constexpr bool is_convertible_to_view_or_qstring_v =
std::is_convertible_v<T, QString> ||
std::is_convertible_v<T, QStringView> ||
std::is_convertible_v<T, QLatin1String>;

// check if we can call QString::number(T) with T
template<typename T>
static constexpr bool is_number_compatible_v =
std::is_invocable_v<decltype(QString::number(std::declval<T>()))(T), T>;

// always false, used for static_assert and workaround for compilers without
// https://cplusplus.github.io/CWG/issues/2518.html
template<typename T>
static constexpr bool always_false_v = false;
} // namespace

// Try to convert T to a type that would be accepted by QString::args(Args&&...)
template<typename T>
auto convertToQStringConvertible(T&& arg) {
if constexpr (is_convertible_to_view_or_qstring_v<T>) {
// no need to do anything, just return verbatim
return std::forward<T>(arg);
} else if constexpr (is_number_compatible_v<T>) {
return QString::number(std::forward<T>(arg));
} else {
static_assert(always_false_v<T>, "Unsupported type for QString::arg");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately this throws an error when picked for main (#12548), dunno if that is to be expected, let alone how to fix it (except using static_assert(false, ...) )
https://github.com/mixxxdj/mixxx/actions/runs/9103424016/job/25025246943?pr=12548

/src/util/stringformat.h: In function ‘auto convertToQStringConvertible(T&&)’:
Error: /src/util/stringformat.h:36:23: error: reference to ‘always_false_v’ is ambiguous
   36 |         static_assert(always_false_v<T>, "Unsupported type for QString::arg");
      |                       ^~~~~~~~~~~~~~
/src/util/stringformat.h:24:23: note: candidates are: ‘template<class T> constexpr const bool {anonymous}::always_false_v<T>’
   24 | static constexpr bool always_false_v = false;
      |                       ^~~~~~~~~~~~~~

Copy link
Member Author

@Swiftb0y Swiftb0y May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its getting confused because always_false_v is also already defined in other headers (likely from #12781). So you should be able to fix it by shuffling some code around. Potentially just putting a single definition in a single header and then using that across both files could be enough

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay, in main this seems to clash with always_false_v declared in midimessage.h (where it is inline constexpr bool)

inline constexpr bool always_false_v = false;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(comment race condition : )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially just putting a single definition in a single header and then using that across both files could be enough

What place would you pick?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it would need to be accessible in both places and should not assume many dependencies, so I'd just put it in a new src/util/ header for now.

// unreachable, but returning a QString results in a better error message
// because the log won't be spammed with all the QString::arg overloads
// it couldn't match with `void`.
return QString();
}
}
75 changes: 31 additions & 44 deletions src/util/timer.h
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
#pragma once

#include <QObject>
#include <QString>
#include <optional>
#include <utility>

#include "control/controlproxy.h"
#include "util/cmdlineargs.h"
#include "util/duration.h"
#include "util/parented_ptr.h"
#include "util/performancetimer.h"
#include "util/qstringformat.h"
#include "util/stat.h"

const Stat::ComputeFlags kDefaultComputeFlags = Stat::COUNT | Stat::SUM | Stat::AVERAGE |
Expand Down Expand Up @@ -52,61 +56,44 @@ class SuspendableTimer : public Timer {

class ScopedTimer {
public:
ScopedTimer(const char* key, int i,
Stat::ComputeFlags compute = kDefaultComputeFlags)
: m_pTimer(NULL),
m_cancel(false) {
if (CmdlineArgs::Instance().getDeveloper()) {
initialize(QString(key), QString::number(i), compute);
// Allows the timer to contain a format string which is only assembled
// when we're not in `--developer` mode.
/// @param compute Flags to use for the Stat::ComputeFlags (can be omitted)
/// @param key The format string for the key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// @param key The format string for the key
/// @param key The format string as QStringLiteral to identify the timer

/// @param args The arguments to pass to the format string
template<typename T, typename... Ts>
ScopedTimer(Stat::ComputeFlags compute, T&& key, Ts&&... args)
: m_timer(std::nullopt) {
if (!CmdlineArgs::Instance().getDeveloper()) {
return; // leave timer in cancelled state
}
}

ScopedTimer(const char* key, const char *arg = NULL,
Stat::ComputeFlags compute = kDefaultComputeFlags)
: m_pTimer(NULL),
m_cancel(false) {
if (CmdlineArgs::Instance().getDeveloper()) {
initialize(QString(key), arg ? QString(arg) : QString(), compute);
// key is explicitly `auto` to allow passing non-QString types such as `const char*`
// its conversion is delayed until after we've checked if we're in developer mode
auto assembledKey = QString(std::forward<T>(key));
Comment on lines +74 to +76
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is obsolete now, we don't want something else that a QString generated form a QStringLiteral.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really because we need a local copy we can assign the result of key.arg(...) to.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance wise, we can rely on CoW here IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, since we're taking an universal reference T&& the type will be QString&& anyways, resulting simply in a move which should be even cheaper.

if constexpr (sizeof...(args) > 0) {
// only try to call QString::arg when we've been given parameters
assembledKey = assembledKey.arg(convertToQStringConvertible(std::forward<Ts>(args))...);
}
m_timer = std::make_optional<Timer>(assembledKey, compute);
m_timer->start();
}

ScopedTimer(const char* key, const QString& arg,
Stat::ComputeFlags compute = kDefaultComputeFlags)
: m_pTimer(NULL),
m_cancel(false) {
if (CmdlineArgs::Instance().getDeveloper()) {
initialize(QString(key), arg, compute);
}
}

virtual ~ScopedTimer() {
if (m_pTimer) {
if (!m_cancel) {
m_pTimer->elapsed(true);
}
m_pTimer->~Timer();
}
template<typename T, typename... Ts>
ScopedTimer(T&& key, Ts&&... args)
: ScopedTimer(kDefaultComputeFlags, std::forward<T>(key), std::forward<Ts>(args)...) {
}

inline void initialize(const QString& key, const QString& arg,
Stat::ComputeFlags compute = kDefaultComputeFlags) {
QString strKey;
if (arg.isEmpty()) {
strKey = key;
} else {
strKey = key.arg(arg);
~ScopedTimer() {
if (m_timer.has_value()) {
m_timer->elapsed(true);
}
m_pTimer = new(m_timerMem) Timer(strKey, compute);
m_pTimer->start();
}

void cancel() {
m_cancel = true;
m_timer.reset();
}
private:
Timer* m_pTimer;
char m_timerMem[sizeof(Timer)];
bool m_cancel;
// nullopt also counts as cancelled
std::optional<Timer> m_timer;
};

// A timer that provides a similar API to QTimer but uses render events from the
Expand Down
4 changes: 2 additions & 2 deletions src/waveform/waveformwidgetfactory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -688,8 +688,8 @@ void WaveformWidgetFactory::notifyZoomChange(WWaveformViewer* viewer) {
}

void WaveformWidgetFactory::renderSelf() {
ScopedTimer t("WaveformWidgetFactory::render() %1waveforms",
static_cast<int>(m_waveformWidgetHolders.size()));
ScopedTimer t("WaveformWidgetFactory::render() %1 waveforms",
m_waveformWidgetHolders.size());

if (!m_skipRender) {
if (m_type) { // no regular updates for an empty waveform
Expand Down
Loading