Use QLatin1String for ConfigKey

[Holzhaus]

We might want to consider switching ConfigKey from QString to QLatin1String as those can be significantly faster apparently and I don't think we need/want Unicode support in COs, do we?

Originally posted by @Swiftb0y in #11980 (comment)

[daschuer]

Yes, a latin 1 string is enough.
We need to be careful not to introduce a regression, since the QLatin1Stimg is only a view to the char array.
A std::string has no implicit sharing.

On the other Hände we can improve the even more of we have the string only one in the text segment of our source.

The "reference" QString lives current on the heap in s_qCOHash. So I would start by refactor this. Instead of a QHash it may base on a

• constexpr QLatin1Stimg for groups
• constexpr QLatin1Stimg for item
• 2 x constexpr integer hash of both
• a pointer to both (they lives in the text segment of the first class that using it)
• maybe a pointer to the pointer pair group+item along with the CO itself.

Than we need only to use that handle only in the engine.
And can replace ChannelHandleAndGroup finally

What do you think?

[daschuer]

In short: Get rid of the owning QHash first.

[Swiftb0y]

Mhmm, I'm not sure how feasible that is then. s_qCOHash is a giant datastructure that lives for the majority of program. How would we ensure that everything in it outlives the the QHash?

Whats the problem with ConfigKey storing std::strings instead? Then let the appropriate API's take QLatin1Strings overloads instead. If frequent copies of the ConfigKeys are an issue for you, make the copy constructor explicit or even delete it. If an object needs the ConfigKey, it should take it by &&, if it only needs to read from it (which is much more frequent IMO), take it by const&, no copies necessary.

[daschuer]

A global or static constexpr lives in the text segment and is always available. The address of it may change only during program starts.

[Swiftb0y]

Yes, but in order to assume that in the QHash, we need to make sure to write an interface that enforces that. Otherwise someone will come along and will add something to that table that will become dangling sooner or later. In Rust this would be easy (just use a &'static lifetime)...

Also, this will essentially force the hashtable to be computed at compile time (at least it would make it possible, could even be quite efficient with a precomputed perfect hash). However, there are other components in Mixxx that currently depend on being able to create CO's dynamically (samplers being crated lazily, skins using CO's for skin-specific state, etc). So I don't know how feasible this would be.

The alternative would be to convert ConfigKey to use std::strings but re-add the CoW semantics by just wrapping them in in a generic CoW class or reimplementing CoW semantics for ConfigKey...

[daschuer]

With Implicit sharing and const references everywhere, I think we are already close to optimum when we consider to avoid copies. I am concerned to reach this state with std::string easy.

On one hand this will save us from the double length of the strings only relevant at initialization. But we have "some" time during startup for that.
Comparisons will perform faster during runtime, but lets reduce them to an address compare!

This is a major benefit during runtime that likely has an effect. A ChannelHandleAndGroup solution for all.

Let's do the work for that and the initialization speed up comes for free.

[Swiftb0y]

This is a major benefit during runtime that likely has an effect. A ChannelHandleAndGroup solution for all.

Please elaborate.

[daschuer]

I think we can do it somehow like this:

namespace {

constexpr char* key1 = "[Main]";
constexpr char* key2 = "[Skin]";

}

constexpr int constexpr_hash_funtion(constexpr char* key) {
...
}

{
    hash_map_with_constexpr_hash<std::string_view, char*, constexpr_hash_funtion> hashTable;

    hashTable[std::string_view(key1)] = {&key1[0], constexpr_hash_funtion(key1)};
    hashTable[std::string_view(key2)] = {&key2[0], constexpr_hash_funtion(key2)};
}

This way we can use "&key1[0]" as handle for comparison later.
In case of a key/item pair we have two of them, or a single handle to the pair.

Also, this will essentially force the hashtable to be computed at compile time

Yes, the dynamic filling will remain. It would be nice to find a solution that supports a constexpr hashes like outlined above.

Extenal code like mappings and skins can look up the strings at runtime or may even compute the hash at compile time.

Do we have hash collisions? If we assert that we do not have them, only the Hash would be sufficient for a lookup.

[Swiftb0y]

This way we can use "&key1[0]" as handle for comparison later.

Not sure if that works, needs experimentation.

Do we have hash collisions? If we assert that we do not have them, only the Hash would be sufficient for a lookup.

If we know the content of the entire table at compile time, we can generate a perfect hash function (see gperf, constexpr-phf, frozen::unordered_map). Static lookups will be cheap as the hash can be derived at compile time. Dynamic lookups would also be cheaper since we know there to be no collisions. Dynamic construction would need a separate table that is independent of the compile-time table.
Whether that avoids collisions, I don't know. Since nothing can be added at compile-time, collision resolution would be unnecessary, but since the set of keys that can be looked up at compile time is not known, there likely still are collisions.

[Swiftb0y]

Regardless, it makes very little sense to discuss this without being to able to measure any potential gain and whether this is necessary in the first place.

[daschuer]

Why do you write in bold? Testing is always a good idea if you wan to improve performance.

The ChannelHandleAndGroup was a mayor performance gain, a similar solution for all will also give some performance. Just moving to std::string may involve a regression due to less optimal compares/hashing because of absence of implicit sharing. I don't think it worth the effort and risk of regressions. Moving to QByteArray cound be a quick win though.

[Swiftb0y]

Why do you write in bold? Testing is always a good idea if you wan to improve performance.

Because I wanted to emphasize that it makes no sense to discuss details of performance implications before we write benchmarks. That is the very first step to improvement.

The ChannelHandleAndGroup was a mayor performance gain, a similar solution for all will also give some performance.

Do we have / did we have benchmarks on that?

[daschuer]

We have manual test results documented here:
7652b56

[Swiftb0y]

Thank you. Then the next step would be to do similar measurements for COs and see whether its worth improving.