Closed 10110111 closed 2 months ago
Great PR! Please pay attention to the following items before merging:
Files matching src/**/*.cpp
:
This is an automatically generated QA checklist based on modified files.
Interesting! I had meanwhile considered using qHash(name) which may also compare faster. Before I run this: are there any dangers now using Latin1? I mean, all our text files including object names are now UTF8. But I think the string compare avoidances in #3847 are here to stay.
Before I run this: are there any dangers now using Latin1? I mean, all our text files including object names are now UTF8.
No, this only affects string literals. And all the places I changed have only Latin1 characters: like "Sun"
, "none"
etc.
(Ideally, we should also edit other cpp files, but this PR only deals with the ones that seem to most frequently appear in the profiler output.)
Ah, I misunderstood the intention of QLatin1String. But according to https://doc.qt.io/qt-6/qlatin1stringview.html I don't expect a change. Will profile a bit later. (Should this StelUtils macro become QLatin1StringView in Qt6 builds?)
I don't expect a change
Yes, that page doesn't make the main point explicit. The point is that QString("asdf")=="bbb"
invokes QUtf8::compareUtf8
, while QString("asdf")==QLatin1String("bbb")
skips the comparison loop early by using the fact that in Latin1 encoding is fixed-length, implying that number of bytes is equal to string length. And even if the lengths of the strings compared coincide, it uses a simple loop in ucstrncmp
instead of doing UTF-8 unpacking magic in QUtf8::compareUtf8
.
Should this StelUtils macro become QLatin1StringView in Qt6 builds?
They might, but I don't think it will noticeably improve performance, since QLatin1String
already avoids making a deep copy of the data. Note the documentation in the constructor from const char*
:
The string data is not copied. The caller must be able to guarantee that str will not be deleted or modified as long as the QLatin1String object exists.
Why not just apply it to #3847 ? I don't even want to profile master again after all these improvements.
Why not just apply it to #3847 ?
This change makes sense regardless of whether we'll accept all of the changes in that PR or only some of them, and I want to avoid cluttering the already hard to analyze diff there even further. I can (and actually will have to) add this commit there too for profiling, but I'm going to merge this one independently if there're no objections.
Hello @10110111!
Please check the fresh version (development snapshot) of Stellarium: https://github.com/Stellarium/stellarium-data/releases/tag/weekly-snapshot
Hello @10110111!
Please check the latest stable version of Stellarium: https://github.com/Stellarium/stellarium/releases/latest
In #3847 we can often see
QUtf8::compareUtf8
appear in the profiler output. This PR makes string comparisons lighter due to the lack of need to convert from UTF-8 to UTF-16 or whatever the comparison function does.The
QLatin1String
function is abbreviated toL1S
by a#define
inStelUtils
, because oftentimes we have multiple string literals in a singleif
statement, for which using the full function name would make the line much longer.This might make some of the changes in #3847 no longer important.