Config object hash

[daschuer]

This replaces the linear lookup code by a qHash.

(Maybe this is a first step towards joining config and control objects)

[rryan]

ConfigObject has always been a giant train-wreck thread-safety wise.

@kain88-de wrote a nice mixxx-devel post arguing for QSettings a while back and I believe he already started work on it:
http://sourceforge.net/p/mixxx/mailman/message/30690555/

https://github.com/mixxxcommunity/mixxx/tree/max-linke-qsettings

[rryan]

(Maybe this is a first step towards joining config and control objects)

I don't think they should be joined -- one is for runtime control processing and one is a representation of the user's preferences.

[kain88-de]

Yes I thought about switching to QSettings. In fact I wanted to start working on that after the release since it will touch almost every file in mixxx.

[daschuer]

Ok, thank you. Than we can merge this to 1.12?
I think it is a nice gain and will not prevent QSettings since we probably continue to use our goup/item pair.

[rryan]

@daschuer, why did you decide to work on this change? Did the linear behavior of QList show up in profiling as problematic?

I'm worried that QHash is more crash-prone when touched across threads than QList. I'm against making a change like this so close to the release unless it is motivated by a good reason.

[daschuer]

I have no profiling results, but the problem shows up during debugging my StringAtom Branch.
The side-chain calls a config value lookup in very process round.
https://github.com/mixxxdj/mixxx/blob/master/src/engine/sidechain/engineshoutcast.cpp#L509

And since I was afraid there are some more regular config value look ups and the solution was so obvious ... that the result.

[rryan]

I can't remember a bug in this code in the past 7 years of working on Mixxx and we haven't touched it much in that time period either. I'm for taking the safe route and not messing with what isn't broken and then making big changes after the release.

[daschuer]

Ok. I thought we are currently optimizing the performance ..

But anyway: there is an other more important issue: The seek errors with mp3

[rryan]

Ok. I thought we are currently optimizing the performance ..

Indeed we are! But performance changes should be motivated by data.

[ywwg]

Indeed we are! But performance changes should be motivated by data.

yeah profiling mixxx is really hard, and I've gone down a few dead-ends in the past looking at functions that, say, show up high in gprof results. RJ pointed out that if my CPU isn't even hitting 100%, xruns are not happening because of inefficient algorithms :). My instincts have been proven wrong a few times.

In the case of the beats-lookup change I made, that's something RJ and I had gone over before, noticing the mutex locking and noticing the inefficiency of calculating the same data twice to get two different pieces of data out of the result. And then I discovered we were doing way way way more beat lookups than were necessary, so I went and did that patch.

There's also a danger in creating artificial benchmarks to justify changes -- if you have to run something X-million times to see a difference, but that code path isn't called very often in mixxx, it's not really helpful.

[rryan]

This won't necessarily group same-group keys together since qHash(ConfigKey) can spread them in different hash buckets. A QMap may be better because its iteration order is sorted by its keys. Or we could group the ConfigKeys by group when saving.

[daschuer]

Nice Idea!
Now it uses QMap, which produces a sorted mixxx.cfg file.
Very convenient.

[daschuer]

OK, now a mutex is in place to protect against the race condition from bug https://bugs.launchpad.net/mixxx/+bug/1428500
.. and it fixes also the crash-prone QHash when touched across threads.

Edit: Fixed bug URL

[daschuer]

Since this branch fixed a race condition. I would like to merge it soon.
Thoughts?

[rryan]

technically a change in behavior?

[rryan]

Coming back to this since we've released. Other than the minor comments, looks good to me -- thanks!

[daschuer]

Thank you for review.

[uklotzde]

It's common practice to implement comparison operators as free functions with left- and right-hand-side parameters (lhs/rhs) of the same type. Just add "friend" and a second parameter here.

[rryan]

Our style guide gives a reason why to prefer it: "Prefer to define non-modifying binary operators as non-member functions. If a binary operator is defined as a class member, implicit conversions will apply to the right-hand argument, but not the left-hand one. It will confuse your users if a < b compiles but b < a doesn't."

https://google.github.io/styleguide/cppguide.html#Operator_Overloading

[uklotzde]

Very good and concise explanation :)

On 01/12/2016 09:41 AM, RJ Ryan wrote:

In src/configobject.h
#501 (comment):

 static ConfigKey parseCommaSeparated(QString key);

 inline bool isNull() const {
     return group.isNull() && item.isNull();
 }

• // comparison function for ConfigKeys. Used by a QHash in ControlObject
• inline bool operator==(const ConfigKey& other) const {

Our style guide gives a reason why to prefer it: "Prefer to define
non-modifying binary operators as non-member functions. If a binary
operator is defined as a class member, implicit conversions will apply to
the right-hand argument, but not the left-hand one. It will confuse your
users if a < b compiles but b < a doesn't."

https://google.github.io/styleguide/cppguide.html#Operator_Overloading

—
Reply to this email directly or view it on GitHub
https://github.com/mixxxdj/mixxx/pull/501/files#r49427091.

[daschuer]

Fixed in: aab96ea