Refactor/modernize shrink scopedtimer

[Swiftb0y]

first commit just modernizes it a bit, and doesn't touch the API, the second commit goes a step further and leverages QString::arg(Args&&...) to provide arbitrary formastring support. let me know what you think.

[Swiftb0y]

build all green. I think its easier to review if I squash all the fixups first. Or do you already have a review pending? @daschuer

[daschuer]

Do we have a use case where we need to pass a QString as key or args? We should not allow this IMHO. The current implementation and the related comments will be tempting preferably use a QString. I think the fastest solution is to use a constexpr QLatin1String

template<typename... Ts>
ScopedTimer(const QLatin1String& key, Ts&&... args) {
    static_assert(std::is_constexpr_v<decltype(key)>);

Alternatinve that does not require ""_L1 but also avoids searching \0 at runtime:

template<std::size_t N, typename... Ts>
ScopedTimer(const char key[N], Ts&&... args) {
    QString k(key, N-1) 

(Both untested)

[Swiftb0y]

Do we have a use case where we need to pass a QString as key or args? We should not allow this IMHO. The current implementation and the related comments will be tempting preferably use a QString.

Well, currently both "QLatin1String" and const char[N] passing works, but because we end up passing the QString key to the Timer, it sooner or later needs to become a proper QString anyways (or at least an owning string type).

[daschuer]

We need to make sure that the QString creation is always delayed until the developer flag is checked to not malloc memory or issue any other locking system call during the normal run.

[Swiftb0y]

Yes, I'm aware. The only way that could happen is if the caller explicitly constructs a the string the wrong way for example by QString("some literal"). Using a literal directly works correctly (and delays construction as intended). I don't want to blacklist passing a QString because that would prevent passing a QStringLiteral.

[daschuer]

Ah, QStringLiteral. This way we get around the UTF-16 conversion. You may check if it is a string literal by assert that the capacity() == 0 but size() > 0

Maybe we create our own macro that tuns existing calls into a QStringLiteral call? This way we can add a private function that accepts "const QString&" only.

[Swiftb0y]

You may check if it is a string literal by assert that the capacity() == 0 but size() > 0

Why would I need to check that? I'd need to do that at compile time for it to be useful.

Maybe we create our own macro that tuns existing calls into a QStringLiteral call?

Something like this?

#define BENCH(key, ...) \
ScopedTimer(QStringLiteral(key), __VA_ARGS__)

That would allow usage like:

ScopedTimer t{BENCH("key")};

[Swiftb0y]

tbh, I'd prefer to avoid the macro and instead construct the literal directly:

// assumes previous `using namespace Qt::StringLiterals`
ScopedTimer t(u"key"_s);

I think the ergonomics are decent enough. The stringliteral is also constructed at compile time once you enable -O1. [Demo](https://compiler-explorer.com/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,selection:(endColumn:21,endLineNumber:4,positionColumn:21,positionLineNumber:4,selectionStartColumn:21,selectionStartLineNumber:4,startColumn:21,startLineNumber:4),source:'%23include+%3CQtCore/QString%3E%0A%0AQString+foo()+%7B%0A++++using+namespace+Qt::StringLiterals%3B%0A++++return+u%22key%22_s%3B%0A%7D'),l:'5',n:'1',o:'C%2B%2B+source+%231',t:'0')),k:33.333333333333336,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((h:compiler,i:(compiler:gsnapshot,filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'1',intel:'0',libraryCode:'0',trim:'1',verboseDemangling:'0'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,libs:!((name:qt,ver:'660'),(name:pcre2,ver:'1042')),options:'--std%3Dc%2B%2B20+-fPIE+-O1+-Wall+-Wpedantic',overrides:!(),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+x86-64+gcc+(trunk)+(Editor+%231)',t:'0')),header:(),k:33.333333333333336,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compilerName:'x86-64+gcc+(trunk)',editorid:1,fontScale:14,fontUsePx:'0',j:1,wrap:'1'),l:'5',n:'0',o:'Output+of+x86-64+gcc+(trunk)+(Compiler+%231)',t:'0')),k:33.33333333333333,l:'4',n:'0',o:'',s:0,t:'0')),l:'2',n:'0',o:'',t:'0')),version:4)

[daschuer]

With the proposed ScopedTimer t(u"key"_s); call, it is IMHO way to easy to use it like ScopedTimer t("key"); which allocates memory at runtime.

You may check if it is a string literal by assert that the capacity() == 0 but size() > 0

Why would I need to check that? I'd need to do that at compile time for it to be useful.

To avoid that users are starting to compose the QString outside the function. This would be DEBUG_ASSERT at runtime. Or is there a way to check it at compile time?

One option would be to hide the ScopedTimer(const QString& key) somehow. That can be done with a macro.

Like

#define ScopedTimer(key, ...) \
ScopedTimerPrivate(QStringLiteral(key), __VA_ARGS__)

Which has the benefit the we need to change nothing in the calling code.

tbh, I'd prefer to avoid the macro and instead construct the literal directly:

I don't see a way around it, because QStringLiteral is also a macro.

[ronso0]

Infamous "Close with comment"?

[Swiftb0y]

With the proposed ScopedTimer t(u"key"_s); call, it is IMHO way to easy to use it like ScopedTimer t("key"); which allocates memory at runtime.

Yes, just as it did previously. but in the latest version, the const char* version only allocates after the devmode check. Also if that fallback is too graceful, we can just static_assert that the result is still a QString. Then the only way to shoot yourself in the foot here is if you explicitly do the QString cast on a literal ( ScopedTimer t(QString("key")), which at this point is an anti-pattern by itself independent of ScopedTimer (clazy's qstring-unneeded-heap-allocations eliminates this and should already be enabled).

I don't see a way around it, because QStringLiteral is also a macro.

but operator""_s isn't. Also any macro would still have to call a public constructor which people could also just call directly.

This would be DEBUG_ASSERT at runtime. Or is there a way to check it at compile time?

Not that I would know. Sure we can assert at runtime, but should that be after or before the devmode check. Before it would reduce in overhead, after it would have a good chance to be missed without compiling with -DDEBUG_ASSERTIONS_FATAL.

[daschuer]

My idea was to put it after the developer mode check. If they introduce the timer, I am sure they will also use it.

Does the ScopedTimer t("key"); version count until \0? Do we want to prevent that because of the UTF-16 conversion? A macro version as discussed above will always use the fastest way and we can disallow all other ways.

[Swiftb0y]

Does the ScopedTimer t("key"); version count until \0?

yes. but only after the check. It probably wouldn't with QT_RESTRICTED_CAST_FROM_ASCII defined. But that might require a bigger change. Also strlen is cheap compared to the required allocation and especially from fromUtf8 conversion. But as I said, we can simply disable construction from const char* if you think thats an issue.

A macro version as discussed above will always use the fastest way and we can disallow all other ways.

We can achieve the same if not more without a macro. Can you restate your exact requirements? I'm sure I can find a macro-less solution.

[daschuer]

OK here is the list:

1. The current solution prevents allocating when not in developer mode by 100%. I think a new solution should do the same to be not considered as regression
2. Optional: Fix implicit use of strlen() at runtime when it loops through the whole string
3. Optional: Fix UTF-16 conversion at runtime

[Swiftb0y]

The current solution prevents allocating when not in developer mode by 100%. I think a new solution should do the same to be not considered as regression

This is already the case.

Fix implicit use of strlen() at runtime when it loops through the whole string.

I assume with "fix" you mean "avoid"? I don't think thats really feasible. The only API that could do that would either be QString::fromUtf8/16 but that would require special case handling which is unnecessarily clunky to deal with.

Optional: Fix UTF-16 conversion at runtime

Not entirely sure how to do that reliably, but I can add a heuristic static_assert that tries to verify that the char type is 16-bit wide. Wdyt?

[daschuer]

I can unfortunately not confirm that it fails after:

-    ScopedTimer t(QStringLiteral("AnalyzerEbur128::processSamples()"));
+    ScopedTimer t("AnalyzerEbur128::processSamples()");

-    ScopedTimer t(QStringLiteral("AnalyzerEbur128::processSamples()"));
+    ScopedTimer t(QString("AnalyzerEbur128::processSamples()"));

[daschuer]

It works as desired, when we replace the first template parameter with const QString&, than delete the const char* variant. The not QStringLiteral can be sorted out by DEBUG_ASSERT(key.capacity() == 0). You may also consider to add an even faster overload without Ts&&

[Swiftb0y]

It works as desired, when we replace the first template parameter with const QString&, than delete the const char*

Yes, thats also an option, but I thought the static_assert results in nicer debug output when violated compared to what the compiler reduces when it matches the deleted ctor.

You may also consider to add an even faster overload without Ts&&

I don't see how that would be faster in terms of runtime. It only adds unnecessary repetition which this PR was supposed to reduce.

[daschuer]

I thought the static_assert results in nicer

Probably yes, but I cannot confirm it is working in the current state. The error output with the deleted function is good enough IMHO because it is no template type.

src/analyzer/analyzerebur128.cpp:53:55: error: use of deleted function ‘ScopedTimer::ScopedTimer(char*, Ts&& ...) [with Ts = {}]’
   53 |     ScopedTimer t1("AnalyzerEbur128::processSamples()");
      |                                                       ^
In file included from src/analyzer/analyzerebur128.cpp:9:
src/util/timer.h:62:5: note: declared here
   62 |     ScopedTimer(char*, Ts&&... args) = delete;
      |     ^~~~~~~~~~~
ninja: build stopped: subcommand failed.

I don't see how that would be faster in terms of runtime.

Oh yes, I missed the fact that if constexpr is optimized away.

[Swiftb0y]

I can unfortunately not confirm that it fails after:

-    ScopedTimer t(QStringLiteral("AnalyzerEbur128::processSamples()"));
+    ScopedTimer t("AnalyzerEbur128::processSamples()");

I cannot confirm it is working in the current state.

whoops, yeah, forgot const char* is not char const*. fixed in fixup

[Swiftb0y]

Oh yes, I missed the fact that if constexpr is optimized away.

Yeah, fortunately not only optimized away but guaranteed to not be called because otherwise trying to call QString::arg without any parameters would be a compile-error (as already pointed out by the comment in the code)

[Swiftb0y]

once this lgty I'll rebase out the unnecessary intermediate commits and squash the fixups

[daschuer]

Now I get this error message. Unfortunately it is like a wall of text not pointing directly to the point of the error.

In file included from src/analyzer/analyzerebur128.cpp:9:
/home/sperry/workspace/mixxx2/src/util/timer.h: In instantiation of ‘ScopedTimer::ScopedTimer(Stat::ComputeFlags, T&&, Ts&& ...) [with T = const char (&)[34]; Ts = {}; Stat::ComputeFlags = int]’:
src/util/timer.h:88:96:   required from ‘ScopedTimer::ScopedTimer(T&&, Ts&& ...) [with T = const char (&)[34]; Ts = {}]’
src/analyzer/analyzerebur128.cpp:54:55:   required from here
src/util/timer.h:68:23: error: static assertion failed: string type likely not UTF-16, please pass QStringLiteral by means of u"key text"_s
   68 |         static_assert(!std::is_same_v<std::decay_t<T>, char const*>,
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[7/38] Building CXX object CMakeFiles/...lib.dir/src/library/dao/trackdao.cpp.o
ninja: build stopped: subcommand failed.

Do you really think this text is easier to understand than just the one demonstrated above: "use of deleted function" with the faulty line of code shown?

[daschuer]

When one uses the ScopedTimer t3(u"AnalyzerEbur128::processSamples()"); version introduced in main https://github.com/mixxxdj/mixxx/blob/8f647908af460e53fe8a860b6ce0964a93a55112/src/analyzer/analyzerebur128.cpp#L52 the error message becomes awful:

In file included from src/analyzer/analyzerebur128.cpp:9:
src/util/timer.h: In instantiation of ‘ScopedTimer::ScopedTimer(Stat::ComputeFlags, T&&, Ts&& ...) [with T = const char16_t (&)[34]; Ts = {}; Stat::ComputeFlags = int]’:
src/util/timer.h:88:96:   required from ‘ScopedTimer::ScopedTimer(T&&, Ts&& ...) [with T = const char16_t (&)[34]; Ts = {}]’
src/analyzer/analyzerebur128.cpp:55:56:   required from here
src/util/timer.h:76:29: error: conversion from ‘const char16_t [34]’ to ‘QChar’ is ambiguous
   76 |         auto assembledKey = QString(std::forward<T>(key));
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h:48,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qhashfunctions.h:44,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h:47,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/QList:1,
                 from src/track/track_decl.h:5,
                 from src/analyzer/analyzertrack.h:5,
                 from src/analyzer/analyzer.h:3,
                 from src/analyzer/analyzerebur128.h:5,
                 from src/analyzer/analyzerebur128.cpp:1:
/usr/include/x86_64-linux-gnu/qt5/QtCore/qchar.h:90:22: note: candidate: ‘constexpr QChar::QChar(char16_t)’ <near match>
   90 |     Q_DECL_CONSTEXPR QChar(char16_t ch) Q_DECL_NOTHROW : ucs(ushort(ch)) {} // implicit
      |                      ^~~~~
/usr/include/x86_64-linux-gnu/qt5/QtCore/qchar.h:90:22: note:   conversion of argument 1 would be ill-formed:
In file included from src/analyzer/analyzerebur128.cpp:9:
src/util/timer.h:76:29: error: invalid conversion from ‘const char16_t*’ to ‘char16_t’ [-fpermissive]
   76 |         auto assembledKey = QString(std::forward<T>(key));
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                             |
      |                             const char16_t*
In file included from /usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h:48,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qhashfunctions.h:44,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h:47,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/QList:1,
                 from src/track/track_decl.h:5,
                 from src/analyzer/analyzertrack.h:5,
                 from src/analyzer/analyzer.h:3,
                 from src/analyzer/analyzerebur128.h:5,
                 from src/analyzer/analyzerebur128.cpp:1:
/usr/include/x86_64-linux-gnu/qt5/QtCore/qchar.h:86:22: note: candidate: ‘constexpr QChar::QChar(int)’ <near match>
   86 |     Q_DECL_CONSTEXPR QChar(int rc) Q_DECL_NOTHROW : ucs(ushort(rc & 0xffff)) {}
      |                      ^~~~~
/usr/include/x86_64-linux-gnu/qt5/QtCore/qchar.h:86:22: note:   conversion of argument 1 would be ill-formed:
In file included from src/analyzer/analyzerebur128.cpp:9:
src/util/timer.h:76:29: error: invalid conversion from ‘const char16_t*’ to ‘int’ [-fpermissive]
   76 |         auto assembledKey = QString(std::forward<T>(key));
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                             |
      |                             const char16_t*
In file included from /usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h:48,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qhashfunctions.h:44,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h:47,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/QList:1,
                 from src/track/track_decl.h:5,
                 from src/analyzer/analyzertrack.h:5,
                 from src/analyzer/analyzer.h:3,
                 from src/analyzer/analyzerebur128.h:5,
                 from src/analyzer/analyzerebur128.cpp:1:
/usr/include/x86_64-linux-gnu/qt5/QtCore/qchar.h:85:22: note: candidate: ‘constexpr QChar::QChar(uint)’ <near match>
   85 |     Q_DECL_CONSTEXPR QChar(uint rc) Q_DECL_NOTHROW : ucs(ushort(rc & 0xffff)) {}
      |                      ^~~~~
/usr/include/x86_64-linux-gnu/qt5/QtCore/qchar.h:85:22: note:   conversion of argument 1 would be ill-formed:
In file included from src/analyzer/analyzerebur128.cpp:9:
src/util/timer.h:76:29: error: invalid conversion from ‘const char16_t*’ to ‘uint’ {aka ‘unsigned int’} [-fpermissive]
   76 |         auto assembledKey = QString(std::forward<T>(key));
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                             |
      |                             const char16_t*
In file included from /usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h:48,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qhashfunctions.h:44,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h:47,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/QList:1,
                 from src/track/track_decl.h:5,
                 from src/analyzer/analyzertrack.h:5,
                 from src/analyzer/analyzer.h:3,
                 from src/analyzer/analyzerebur128.h:5,
                 from src/analyzer/analyzerebur128.cpp:1:
/usr/include/x86_64-linux-gnu/qt5/QtCore/qchar.h:84:22: note: candidate: ‘constexpr QChar::QChar(short int)’ <near match>
   84 |     Q_DECL_CONSTEXPR QChar(short rc) Q_DECL_NOTHROW : ucs(ushort(rc)) {} // implicit
      |                      ^~~~~
/usr/include/x86_64-linux-gnu/qt5/QtCore/qchar.h:84:22: note:   conversion of argument 1 would be ill-formed:
In file included from src/analyzer/analyzerebur128.cpp:9:
src/util/timer.h:76:29: error: invalid conversion from ‘const char16_t*’ to ‘short int’ [-fpermissive]
   76 |         auto assembledKey = QString(std::forward<T>(key));
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                             |
      |                             const char16_t*
In file included from /usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h:48,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qhashfunctions.h:44,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h:47,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/QList:1,
                 from src/track/track_decl.h:5,
                 from src/analyzer/analyzertrack.h:5,
                 from src/analyzer/analyzer.h:3,
                 from src/analyzer/analyzerebur128.h:5,
                 from src/analyzer/analyzerebur128.cpp:1:
/usr/include/x86_64-linux-gnu/qt5/QtCore/qchar.h:82:22: note: candidate: ‘constexpr QChar::QChar(ushort)’ <near match>
   82 |     Q_DECL_CONSTEXPR QChar(ushort rc) Q_DECL_NOTHROW : ucs(rc) {} // implicit
      |                      ^~~~~
/usr/include/x86_64-linux-gnu/qt5/QtCore/qchar.h:82:22: note:   conversion of argument 1 would be ill-formed:
In file included from src/analyzer/analyzerebur128.cpp:9:
src/util/timer.h:76:29: error: invalid conversion from ‘const char16_t*’ to ‘ushort’ {aka ‘short unsigned int’} [-fpermissive]
   76 |         auto assembledKey = QString(std::forward<T>(key));
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                             |
      |                             const char16_t*
In file included from /usr/include/x86_64-linux-gnu/qt5/QtCore/qhashfunctions.h:44,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/qlist.h:47,
                 from /usr/include/x86_64-linux-gnu/qt5/QtCore/QList:1,
                 from src/track/track_decl.h:5,
                 from src/analyzer/analyzertrack.h:5,
                 from src/analyzer/analyzer.h:3,
                 from src/analyzer/analyzerebur128.h:5,
                 from src/analyzer/analyzerebur128.cpp:1:
/usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h:225:19: note:   initializing argument 1 of ‘QString::QString(QChar)’
  225 |     QString(QChar c);
      |             ~~~~~~^
[7/34] Building CXX object CMakeFiles/...src/skin/legacy/legacyskinparser.cpp.o
ninja: build stopped: subcommand failed.

[Swiftb0y]

Do you really think this text is easier to understand than just the one demonstrated above: "use of deleted function" with the faulty line of code shown?

I'd say so yes because the solution with the deleted overload just says "use of deleted function" with no clear guidance on how to solve the issue. In the static_assert text we can provide an explanation and guidance on how to fix it. Yeah its a little more to read because the error is not on the first line for some reason, but IMO its still easy enough to spot where the error is when you look for the "error: [...]" line.

When one uses the ScopedTimer t3(u"AnalyzerEbur128::processSamples()"); version introduced in main

Ah, I forgot about that PR. the changes in main are very much incompatible with the changes made here anyways. I'll rebase on main then.

[daschuer]

I would prefer the deleted function with an explaining source code comment, but I will not insists.

[Swiftb0y]

Superseeded by #13258