UI: replace QString() with QStringLiteral() for better performance

[Integral-Tech]

Description

• QStringLiteral() provides better performance than QString() when constructing QString objects.

Motivation and Context

• Qt provides a macro QStringLiteral(), which makes constructing QString objects from string literals more efficient.

How Has This Been Tested?

Types of changes

Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)

Checklist:

• My code has been run through clang-format.
• I have read the contributing document.
• My code is not on the master branch.
• The code has been tested.
• All commit messages are properly formatted and commits squashed where appropriate.
• I have included updates to all appropriate documentation.

[Fenrirthviti]

Can you please provide more information on what situations you were seeing a performance issue that lead to this change?

Also what testing was done to show this had improved performance?

[Integral-Tech]

Can you please provide more information on what situations you were seeing a performance issue that lead to this change?

Also what testing was done to show this had improved performance?

Qt Documentation says

Using QStringLiteral instead of a double quoted plain C++ string literal can significantly speed up creation of QString instances from data known at compile time.

From my perspective, the actual difference is not apparent (slightly faster)

[RytoEX]

can significantly speed up creation of QString instances from data known at compile time.

The key part here is "data known at compile time". Most of these instances have .arg data, which is not known at compile time. For returned values, do the function signatures also need to change? Do the call locations implicitly cast the returned value?

[Integral-Tech]

can significantly speed up creation of QString instances from data known at compile time.

The key part here is "data known at compile time". Most of these instances have .arg data, which is not known at compile time. For returned values, do the function signatures also need to change? Do the call locations implicitly cast the returned value?

.arg() function call doesn't matter. With QStringLiteral(), the QString objects are constructed from string literals during compilation.

Function signatures doesn't need to change, as the generated type of QStringLiteral() is still QString.

[Lain-B]

Pretty much all of these are cold paths rather than hot paths. I don't necessarily disagree with doing it but I'm just noting that it's not really all that significant to have occasional allocations in a cold path. Not sure if it's worth the commit but I don't particularly feel strongly about it one way or the other.

[PatTheMav]

I think the important distinction of this PR is to use QStringLiteral for the format templates which are static by design and used with the arg method to generate a final QString at runtime (which always creates a copy of the original QString).

It would be similar to storing a template literal like "My String with argument %1" as a constexpr std::string_view and then using this literal as input for a String format function. The specific benefit is that the string will use the same encoding at compile time as QString does at runtime.

Some more explanation about its benefits can be found here: https://woboq.com/blog/qstringliteral.html

[norihiro]

I compared the time of OBSBasicStats::Update by adding profile_start and profile_end and found no significant improvement.

Below is the measurement with ~1000 calls of OBSBasicStats::Update.

Environment	master	this PR
GCC on Linux	median=0.315 ms, 99th percentile=0.468 ms	median=0.349 ms, 99th percentile=0.517 ms
Clang on Linux	median=0.328 ms, 99th percentile=0.51 ms	median=0.319 ms, 99th percentile=0.492 ms

Also the PR needs to follow the commit guideline. The modification should be separated for each module such as UI, obs-vst, etc. and the commit title should start with the module name.

[Integral-Tech]

I compared the time of OBSBasicStats::Update by adding profile_start and profile_end and found no significant improvement.

Below is the measurement with ~1000 calls of OBSBasicStats::Update.
Environment master this PR
GCC on Linux median=0.315 ms, 99th percentile=0.468 ms median=0.349 ms, 99th percentile=0.517 ms
Clang on Linux median=0.328 ms, 99th percentile=0.51 ms median=0.319 ms, 99th percentile=0.492 ms

Also the PR needs to follow the commit guideline. The modification should be separated for each module such as UI, obs-vst, etc. and the commit title should start with the module name.

When making code refactoring PR, each module should be made separately?

[PatTheMav]

I compared the time of OBSBasicStats::Update by adding profile_start and profile_end and found no significant improvement.
Below is the measurement with ~1000 calls of OBSBasicStats::Update.
Environment master this PR
GCC on Linux median=0.315 ms, 99th percentile=0.468 ms median=0.349 ms, 99th percentile=0.517 ms
Clang on Linux median=0.328 ms, 99th percentile=0.51 ms median=0.319 ms, 99th percentile=0.492 ms
Also the PR needs to follow the commit guideline. The modification should be separated for each module such as UI, obs-vst, etc. and the commit title should start with the module name.

When making code refactoring PR, each module should be made separately?

Yes, at the very least for each "part" of the program, i.e. separate commits for UI, plugins, libobs, etc.

I think it's important to point out that using QStringLiteral might not be a meaningful performance optimisation on the machines we commonly use these days except for heavy loops that might experience runaway runtime cost for each copy operation done within the loop body. Because QString (just as many other Qt classes) is implicitly shared, additional copies are automatically generated if any mutation takes place and the reference count is larger than 1.

QStringLiteral creates a const QString with a reference count of -1, so it can be shared (even across threads) but not mutated (as it does usually indeed end up in the .rodata section of a binary).

So in the same way we use #define all over the codebase (instead of static const char* or constexpr unfortunately) to use string literals, we should be using QStringLiteral for them in Qt code, if only to make it explicit to the reader that these strings should not be changed (and ensure they cannot be changed).

Any performance benefits are just icing on the cake.

[Integral-Tech]

I compared the time of OBSBasicStats::Update by adding profile_start and profile_end and found no significant improvement.
Below is the measurement with ~1000 calls of OBSBasicStats::Update.
Environment master this PR
GCC on Linux median=0.315 ms, 99th percentile=0.468 ms median=0.349 ms, 99th percentile=0.517 ms
Clang on Linux median=0.328 ms, 99th percentile=0.51 ms median=0.319 ms, 99th percentile=0.492 ms
Also the PR needs to follow the commit guideline. The modification should be separated for each module such as UI, obs-vst, etc. and the commit title should start with the module name.

When making code refactoring PR, each module should be made separately?

Yes, at the very least for each "part" of the program, i.e. separate commits for UI, plugins, libobs, etc.

I think it's important to point out that using QStringLiteral might not be a meaningful performance optimisation on the machines we commonly use these days except for heavy loops that might experience runaway runtime cost for each copy operation done within the loop body. Because QString (just as many other Qt classes) is implicitly shared, additional copies are automatically generated if any mutation takes place and the reference count is larger than 1.

QStringLiteral creates a const QString with a reference count of -1, so it can be shared (even across threads) but not mutated (as it does usually indeed end up in the .rodata section of a binary).

So in the same way we use #define all over the codebase (instead of static const char* or constexpr unfortunately) to use string literals, we should be using QStringLiteral for them in Qt code, if only to make it explicit to the reader that these strings should not be changed (and ensure they cannot be changed).

Any performance benefits are just icing on the cake.

I just shrank the scope of this PR to UI only

[Lain-B]

I'll defer to Pat on this one.

[Integral-Tech]

@Lain-B Any progress on this PR? :)

[PatTheMav]

@Lain-B Any progress on this PR? :)

Conceptually I'm fine with the replacement, but I haven't had time to fully review it locally yet (which is mainly for me to give a proper approval). Pending any weirdness I might find it can probably be merged after the holidays (as we just released a new version, need to monitor its progress and then probably want to spend some time with friends and family).

[gxalpha]

This PR only addresses explicit casts from const char to QString. There are tons and tons of implicit casts in the codebase that have the same problems as Pat outlined above, just hidden away from the developers by implicit casting. If people are pedantic enough to merge this PR, I would suggest also considering setting QT_NO_CAST_FROM_ASCII and addressing the implicit casts* - as-is, in my personal opinion this PR doesn't really solve anything.

*As a warning, I do mean tons and tons. In my case the compiler stopped due to too many errors after about half a file. I personally do not think it's worth doing that, but I think the same about this PR (they're the same thing, just explicit vs implicit).