Event queue compressing proxy

[JoergAtGithub]

This PR implements a event queue compressing proxy, which skips the processing of superseded events. This is used in ControlObjectScript.
The PR also contains a unit test which checks the compressing behavior in ControlObjectScript.

What does this compressor solve:
-The compressor prevents unresponsive controllers in CPU overload conditions, when more signals are send from Mixxx, than the mapping can handle. This occurs with complex controllers (with many connected COs) and a not so powerful computer. Especially with signals that appear in high rate, like VU-Meter, Beat indication or Play position.

Behavior without this PR:
-Events are queued and processed as fast as the mapping (or the controller hardware) can
-Status LEDs on controler show oudated states from the queue
-Because controller input and output are handled in the same mapping thread. In overload conditions the mapping also queue the processing of the events send from the controller to the mapping.

Behavior with this PR:
-As long as no CPU overload occurs everything behaves as before
-When an CPU overload occurs, the compressing proxy recursive parse the event Qt event queue vor signals of the same connection. In case of multiple events only the latest is send to the JavaScript callback function in the mapping.
-Therefore status LEDs show always the latest state, and no superseded states from the event queue
-The input is not directly affected, but due to the limited load for the mapping thread, there is always time to process the input events and there are no noticeable delays anymore, between pressing a button and the response of Mixxx.

[JoergAtGithub]

@daschuer wrote:

I also wonder if it is correct to use the proxy unconditional. I can imagine situations where the order of the command is relevant. With this proxy solution the most recent value of the current CO is processed, before other values for other proxys are processed. I am not sure if this is only a theoretical issue or if this can bite use later.

Maybe we should enable this only for value like COs like Playposition and not for command like COs like play or such.

I also thought about this. Such race conditions occur already nowadays. For example, if you use a function like engine.getValue, inside of a mapping function connected to an event. Than you don't know how long the event was already in the queue and how old this value is. The likelihood of these race conditions should be lower with this PR, because the events are not so long in the queue.

I aggree that now a slightly different kind of race conditions can occur, but only if the event queue is filled with multiple events from the same CO but different values, and when the mapping relies on the new value of another CO event, which is in the queue at the same time.

[daschuer]

Such race conditions occur already nowadays. ...

Yes you are probably right. In the case of the engine -> controller connection, I cannot think of a condition where such race condition is a problem. However the engine.getValue() issue cannot be compared, because it is the call to get the current value. When you subscribe the change event you expect to receive ALL changes.
This is no longer guaranteed after merging this.

From this it would be feel better to have this feature conditional.
Did you consider how we can achieve this? Something like a <sequencial/> option?

[uklotzde]

Please don't comment each and every change. This causes many disturbing e-mail notifications. Mark the comments as resolved and request re-review when done.

[JoergAtGithub]

@uklotzde Ok, I will resolve single changes without unecesarry comment in future! Sorry for spamming you!
Correction: I've all needed access rights

[uklotzde]

Code seems to be valid now. Not sure if the changed behavior could break controller scripts?

@daschuer?

[daschuer]

Yes the code is OK. However I am unsure about the implications. In general I think this is a good solution and would like to merge this after we have clarified them.

On one Hand this PR keep Mixxx going in a live situation by "just continuing". We assume that the issues due this PR re less important than the stalled controller, or a message cue overflow. I agree to that.

On the other hand this is almost like the famous vba:

On Error Resume Next

Once this code is committed we have no chance to recognize this cue overload situation and fix the root cause, the inefficient mapping.

@JoergAtGithub Do you have a controller mapping where this code will take action regularly?

Do you think it is a good idea to print debug messages in developer mode if this happens?

Did you consider my comment #4566 (comment)

[JoergAtGithub]

I implemented warnings, which reports when event compressing was done.
In my case with the Traktor Z2 only four different controls were reported VuMeter, VuMeterL, VuMeterR and beat_active. Which are the signals that occur in the highest rate (beat_active only in case of fractional loops).

I expect this occurs in the same way for other controls, which I don't use in my mapping - but only controls which are repeated in high rate are relevant here. Maybe a whitelist of control keys could be implemented and used in ControlObjectScript::addScriptConnection, to decide when the compressing proxy should be connected.

Important to notice is, that a mapping developer might never see these warnings, if his computer is fast enough. But other users of the mapping might expirience an unusable lagging Mixxx (without this PR), with the same mapping.

[JoergAtGithub]

I added an optional argument to the function makeConnection, that needs to be set to true, to enable the compressing proxy.

    engine.makeConnection("[Master]", "VuMeterL", myCallback1); // No compressing proxy
    engine.makeConnection("[Master]", "VuMeterL", myCallback2, false); // No compressing proxy
    engine.makeConnection("[Master]", "VuMeterL", myCallback3, true); // Compressing proxy enabled

I will create a follow-up PR to mass-edit the makeConnections for the critical COs VuMeter, VuMeterL, VuMeterR and beat_active in all JS controller mappings. None of these COs should be used for application logic under normal circumstances.

[daschuer]

Wow, now this feature is complete, right? Thank you for going the extra mile.

[JoergAtGithub]

Yes, I think everything is done now!

[daschuer]

The code looks good.

Just an idea: Maybe you should consider to replace the anonymous bool parameter by an overload function:
engine.makeCompressedConnection("[Master]", "VuMeterL", myCallback1);
Or such.

[JoergAtGithub]

I can do!

[daschuer]

Thank you very much! LGTM

[JoergAtGithub]

@uklotzde @daschuer Thank you, for the good review! I learned a lot!

[Swiftb0y]

@JoergAtGithub Can you write up some docs on how and when to use the new method?

[JoergAtGithub]

@Swiftb0y I can do, but where? The closest place I found is this section:

[Swiftb0y]

Ideally the Wiki, but that is currently read-only for non-coreteam-accounts because of spammers. I'd propose to just open a PR with a markdown file and we'll review that and then instead of merging, I'll copy it to where it belongs in the wiki.

[Holzhaus]

Ideally the Wiki

Ideally we'd have proper documentation how to create JavaScript controller mappings in our manual and reserve the wiki for stuff that is not directly related to Mixxx (e.g. How to set up your IDE, etc.) 😁