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

Integral-Tech commented 3 weeks ago

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:

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

Fenrirthviti commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

I'll defer to Pat on this one.

obsproject / obs-studio