CO hash join and memory leak fix

[daschuer]

In the original version, the memory for ControlDoublePrivate where not feed. I have fixed this by the use of QSharedPointer. I have also removed the hash table for the ControlObject and stored the pointer of the creating ControlObject into ControlDoublePrivate instead.

[rryan]

Hey Daniel -- I'd prefer to have ControlDoublePrivate's be long lived (once created, never destroyed until Mixxx shuts down). That way you can create a CO, delete it, and then multiple COTs can be created to refer to the control after the CO has been deleted (and still get the intended behavior of the control enforced by its ControlBehavior). This is nice because the life-cycle of the control value is not dependent on the life cycle of the code that created it.

[rryan]

Really don't like this since it couples our new system (CDP) with the old, deprecated system (CO). In the future we will have creators which are not COs, right?

[daschuer]

Yes, we have to decide where to go in the next steps. This is may be only an intermediate solution. But for this patch, this was the best way to get rid of the extra hash table in CO.
My fist enthusiasms to make the control object infrastructure cristal clean is gone, because there are so much special cases in the existing code. My current strategy is the one of small re-viewable steps, because it is nearly impossible to test all ~4000 controls on each iteration. Changing the nature of CO is to hot for me yet.

[daschuer]

Thank you for review.

Yes, thats one possible strategy. I have no objections to do so, but I think we should be clear about our concept and the role of each object created.

Currently, we have per key the following:

• One ControlDoublePrivate, holding the value, but not public usable.
• One ControlObject as creator. should live as long as CDP
• Many ControObjectThreads, Life as long as needed.

So if we now allow to delete the CO and let the CDP live, it is somehow like zombie, because you can create COTs but ControlObject::getControl() will fail. IMHO a not a desirable situation.

Strictly following the only create, never delete model, we should allow to create CDP directly and get rid of CO creation capacity and its ControlObject::getControl(). But this will required an other huge round of refactoring.

In Mixxx 1.11, the CO keys are also deleted if the creating objects deletes the CO. We made already use of COs with unlimited lifetime by not deleting them. (They where deleted on Mixxx shutdown). My patch just restores this behavior, with the only exception that it is still possible to communicate via COTs if the creating CO is already deleted. This small change was required to avoid segfaults at Mixxx shutdown because of pending COT updates.

[daschuer]

This patch is starting to rod. I would like to resolve the conflicts and commit it, if there are no further comments or objections.

[rryan]

Sorry for the long delay and rotting :).

After this patch, it's no longer possible to create a CDP without a CO since you can't call getControl with bCreate true. You have to provide a CO to CDP::getControl if you want to create a CDP. This couples CO to CDP -- but CO is deprecated and the new control system should not depend on it at all.

I also still think my comments from earlier are valid. The CDP should live forever. As a weak pointer it will be deleted if nobody else has a reference. This means the value of the CDP will be lost the moment the last person deletes their QSharedPointer to it but we have a getControl method for explicitly re-fetching a strong reference to it.

I'm imagining a world (after we are done with ControlObject) where multiple consumers of a CDP can get it via getControl() and simply set its value without storing a reference to it. This way different agents can use a CDP to share a value (similar to software transactional memory) without having to hold a reference to anything other than the control name.

As a practical example, consider a script environment where we allow script writers to create controls (we've been talking about that for a long time). It would be annoying if the script writer had to make sure they were holding a reference to the control they created in order for it to not be deleted. It would probably be unexpected behavior if this happened:

getControl("vci400.shift_button").set(1);
var shift_status = getControl("vci400.shift_button").get();

// shift_status is 0 because in between the two lines the CDP was deleted because the script writer did not write the code such that a reference to the CDP was held.

[daschuer]

Hi RJ,

Thank you for your comment. You make valid points and I like the idea to simplify the control system much more.

I have committed this patch anyway because it just fixes issues in the current philosophy, pointed out in my previous comment. For me this, together with my other pending patches, is a good base for introducing a philosophy you pointed out.

I am afraid a discussion about new co features will be last beyond a planed 1.12 release because of all the unmerged branches, like 4 Deck, Key shift, Library deblocking, Waveform dejerk and so on. What will be the right place to collect the requirements for a new philosophy?

I am currently preparing an other co patch, which fixes the odd CO warnings in the mixxx.log.

Kind regards,

Daniel