split up ControllerEngine #2920

Be-ing · 2020-07-06T07:55:37Z

This PR breaks ControllerEngine into several classes. ControllerScriptEngineBase is responsible for executing scripts. ControllerScriptModuleEngine subclasses ControllerScriptEngineBase and implements the new JS module system. ControllerScriptEngineLegacy subclasses ControllerScriptEngineBase and implements the old JS/XML system. ControllerScriptInterface is the engine object in the JS environment and replaces ControllerEngineJSProxy.

uklotzde · 2020-07-06T10:12:12Z

Please mark all PRs that are based on other, unfinished PRs as draft.

Be-ing · 2020-07-06T19:36:00Z

I have separated the legacy ControllerEngine code from the new code for JS modules and created a common base class for the shared functionality. This forced removing the code added in #2868 to use JS modules with the legacy XML system. As a next step, I propose separating code for the legacy XML system from the Controller class. Then we can build a new system using a new metadata format and ControllerScriptModuleEngine.

* Move src/controllers/engine to src/controllers/scripting * Create src/controllers/scripting/legacy subfolder * Rename ControllerEngine to ControllerScriptHandler * Rename ControllerEngineJSProxy to ControllerScriptInterface

This allows for a clean separation between new and legacy code.

src/controllers/scripting/controllerscriptenginebase.cpp

daschuer · 2020-07-12T20:38:35Z

src/controllers/scripting/controllerscriptenginebase.cpp

+    m_scriptWatcher.removePaths(m_scriptWatcher.directories());
+
+    // Delete the script engine, first clearing the pointer so that
+    // other threads will not get the dead pointer after we delete it.


And what if the other threads have a pointer already?

Good question. This is legacy code. I have not changed it from ControllerEngine.

I think we need to get rid of such unsafe legacy and not copy it to a new class.
For a similar problem I have invented this:
#1713

AFAIK this has never caused a problem, so I'm not going to fix something that isn't broken.

Please add a TODO. It IS broken, but I agree that we can postpone a solution until is actually causes issues.

I am not adding a TODO comment without a plan for what to do. I don't know what a better solution would be and I'm not discussing it here.

We have here a misleading comment. This needs to be improved at least.

Then please suggest what the comment should say or make a new PR after merging this.

Replace the comment with:
// TODO: We may have handed out the pointer before. Make sure that no other is using it anymore.

I have just removed the misleading comment and replaced the deleteLater with a plain delete. m_pJSEngine is never passed to another thread; this is easy to verify. If a QJSEngine was accessed from different threads there would probably be an obvious crash or other serious bug before this code deleting it is reached.

daschuer · 2020-07-12T20:44:12Z

src/controllers/scripting/controllerscriptenginebase.cpp

+#endif
+}
+
+QJSValue ControllerScriptEngineBase::byteArrayToScriptValue(


This function looks like as it is on the hot path but has two probably expensive calls and a temporary object.
I am sure we can do it better.

I have looked into the APIs for QByteArray, QJSEngine, QJSValue and JS ArrayBuffer but I cannot think of any way to optimize this. Do you have any suggestions?

By the way this is not new. It was added in #1795.

For comparison, here are the legacy functions in the 2.3 branch:

bool ControllerEngine::execute(QScriptValue functionObject, unsigned char channel, unsigned char control, unsigned char value, unsigned char status, const QString& group, mixxx::Duration timestamp) { Q_UNUSED(timestamp); if (m_pEngine == nullptr) { return false; } QScriptValueList args; args << QScriptValue(channel); args << QScriptValue(control); args << QScriptValue(value); args << QScriptValue(status); args << QScriptValue(group); return internalExecute(m_pEngine->globalObject(), functionObject, args); } bool ControllerEngine::execute(QScriptValue function, const QByteArray data, mixxx::Duration timestamp) { Q_UNUSED(timestamp); if (m_pEngine == nullptr) { return false; } QScriptValueList args; args << m_pBaClass->newInstance(data); args << QScriptValue(data.size()); return internalExecute(m_pEngine->globalObject(), function, args); }

This relied on the legacy qtscript-bytearray library, in which the implementation was:

QScriptValue ByteArrayClass::newInstance(const QByteArray &ba) { QScriptValue data = engine()->newVariant(QVariant::fromValue(ba)); return engine()->newObject(this, data); }

QByteArray becomes an ArrayBuffer: https://doc.qt.io/qt-5.12/qtqml-cppintegration-data.html#qbytearray-to-javascript-arraybuffer

Can't we just use the ArrayBuffer and use new Uint8Array(arg1) in the called code? For my understanding this is only cast without copying the data around. This way we get rid of the extra context switch into JS and back.
The caller can decide if it needs the access by index.

Alternatively we may wrap the user callback into another js function that does the cast for convenience.

I would prefer passing the pure ArrayBuffer, because the index access is also not very maintainable.
In the long run, I think we will provide special casting functions which allow named access to the fields in the wire protocol directly.

The open question is whether it is faster to convert a QVector<uint8_t> to a JS Array or convert a QByteArray to a JS ArrayBuffer then convert the ArrayBuffer to a Uint8Array.

I am doubtful it would make (much) of a difference either way. But replacing QByteArray with QVector<uint8_t> in C++ would make the C++ code simpler than wrapping every JS callback into one that converts an ArrayBuffer to a Uint8Array.

I'm still not quite sure whether this makes sense. Using an ArrayBuffer has quite a performance benefit compared to using arrays in JS (assuming you know the size of the container upon creation). At least thats what I gathered from my tests in NodeJS. And when it comes to more complex data with HID for example and access via DataView is required one would have to resort to atrocities like this new DataView(Uint8Array.from(<array here>).buffer);

I think you are right that the QByteArray route would be better. I tried this and realized using QVector requires a deep copy when interfacing with C libraries that use old fashioned C arrays such as HIDAPI. The current code still does create a deep copy, but it should be using QByteArray::fromRawData which is for explicitly not creating a deep copy. I hope QByteArray would not create a deep copy when converted to a JS ArrayBuffer. As far as I understand, that is the whole purpose of ArrayBuffers in JS:

Because a Typed Array is backed by raw memory, the JavaScript engine can pass the memory directly to native libraries without having to painstakingly convert the data to a native representation. As a result, typed arrays perform a lot better than JavaScript arrays for passing data to WebGL and other APIs dealing with binary data.

https://www.html5rocks.com/en/tutorials/webgl/typed_arrays/

mostly, yes. And the way arraybuffers work in JS is also nice because they allow you have different views (sizes; endianess of ints; scopes) on the underlying arraybuffer without ever creating another copy (though there are methods that do, but they have that behavior clearly documented). The only thing thats not so nice is when you want to append multiple arraybuffers because they are fixed in size, you need to create a new buffer that is big enough and then copy them into the new buffer using TypedArray.prototype.set()

Alternatively we may wrap the user callback into another js function that does the cast for convenience.

I have implemented this in e25bf12. It turned out to be quite easy to implement. I am glad I tried the QVector<uint8> idea though because I have discovered unnecessary deep copying in the Controller hardware I/O code.

src/controllers/scripting/controllerscriptmoduleengine.cpp

src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp

src/controllers/controller.cpp

This avoids calling two separate JS functions (one to convert the ArrayBuffer to a Uint8Array then the callback) every time controller input is received.

Be-ing · 2020-12-01T04:16:29Z

Ready for merge again.

…factoring

Be-ing · 2020-12-08T07:26:08Z

Please don't make me keep resolving merge conflicts on this finished branch.

daschuer

There are two pending review comments blocking the merge from my side.

daschuer · 2020-12-09T23:39:15Z

src/control/controlobjectscript.cpp


 ControlObjectScript::ControlObjectScript(const ConfigKey& key, QObject* pParent)
-        : ControlProxy(key, pParent, ControlFlag::NoAssertIfMissing) {
+        : ControlProxy(key, pParent, ControllerDebug::shouldAssertForInvalidControlObjects()) {


I can't follow your explaination.
If a script calls getValue for a non existing CO, Mixxx crashed now.
That must not happen, because we consider a script as user data.
Please revert.

daschuer · 2020-12-09T23:41:55Z

src/controllers/scripting/controllerscriptenginebase.cpp

+                tr("You can ignore this error for this session but "
+                   "you may experience erratic behavior.") +
+                QString("<br>") +
+                tr("Try to recover by resetting your controller.");


Ping! My initial confident has proved that the comment is missing.
This is one main topic in a review. Check if any code is understandable.

ninomp · 2020-12-09T23:50:21Z

I noticed that this PR adds an empty mixxx.log.1 file to the root of repository. I suppose this is a mistake.

Holzhaus

Ok

Be-ing · 2020-12-11T10:27:44Z

Ping! My initial confident has proved that the comment is missing.

Are you seriously going to keep holding this PR back even more months over a trivial code comment that adds nothing?

…factoring

Be-ing · 2020-12-11T10:35:16Z

If every step of the the new controller system will be held back months over disagreements of trivial minutia, I will not continue with it. I just pushed merge conflict resolution. It is the last time I will do it on this branch. If more conflicts develop before someone presses merge, I refuse to continue on this project.

uklotzde · 2020-12-11T10:46:07Z

@daschuer @Holzhaus ? We can continue polishing after this PR has been merged.

Be-ing · 2020-12-11T10:46:18Z

Whether to add the code comment or not is trivial. What is not trivial is creating an environment where developers are continually frustrated that everything moves so slowly.

Be-ing · 2020-12-11T10:47:02Z

We can continue polishing after this PR has been merged.

This PR has been polished multiple times already. I even polished legacy code I had no intention of touching.

daschuer · 2020-12-11T14:40:54Z

Ping! My initial confident has proved that the comment is missing.

Are you seriously going to keep holding this PR back even more months over a trivial code comment that adds nothing?

Unfortunately my review comments do not become void only because the original developers do not respond. Is there really a point of letting a PR rod only because you think requested comment is pointless?

daschuer · 2020-12-11T14:45:05Z

Whether to add the code comment or not is trivial. What is not trivial is creating an environment where developers are continually frustrated that everything moves so slowly.

That can be easily fixed, by being more open to review comments. Sometimes things are harder to solve, but not responding and being frustrated is also not a solution.

daschuer · 2020-12-11T14:50:14Z

I noticed that this PR adds an empty mixxx.log.1 file to the root of repository. I suppose this is a mistake.

This is also pending.

Be-ing · 2020-12-11T16:32:14Z

I will not go in circles repeating myself for weeks. I've done that too many times already.

uklotzde · 2020-12-11T17:32:02Z

I would like to invite everyone to come back to the table after taking a deep breath.

Removing the accidentally committed log file is a valid claim, I didn't notice this.

Couldn't we discuss and resolve the remaining issues and concerns afterwards in subsequent follow-up PRs? They don't seem to be essential to me. This PR is targeting a development version and the changes are just the starting point. No need for final polishing imho.

Be-ing · 2020-12-11T17:35:18Z

Sorry, I misread @ninomp's comment as saying that this PR generates an inappropriately placed log file at runtime. I removed the accidentally committed file.

Be-ing · 2020-12-11T17:41:11Z

Polishing is not the issue. This is beyond polished. It improves code that is soon to be deprecated. I am now being asked to clutter code I did not write with a useless comment that says nothing the code already says itself.

Be-ing · 2020-12-15T15:24:55Z

There are conflicts again. I refuse to fix them. If anyone else thinks this project is more important than adding noise to the code with an inane comment, I leave it to you to fix the conflicts.

uklotzde · 2020-12-15T16:02:41Z

There are conflicts again. I refuse to fix them. If anyone else thinks this project is more important than adding noise to the code with an inane comment, I leave it to you to fix the conflicts.

This is really unfortunate :((

uklotzde marked this pull request as draft July 6, 2020 10:11

Be-ing requested a review from Holzhaus July 6, 2020 13:53

Be-ing force-pushed the controllerengine_refactoring branch 3 times, most recently from facc601 to a5bdd06 Compare July 6, 2020 20:50

Be-ing mentioned this pull request Jul 9, 2020

add support for loading controller scripts as JS modules #2868

Merged

4 tasks

Be-ing force-pushed the controllerengine_refactoring branch 2 times, most recently from 37dd700 to 189604d Compare July 10, 2020 10:32

Be-ing added this to the 2.4.0 milestone Jul 12, 2020

Be-ing force-pushed the controllerengine_refactoring branch from 189604d to b619006 Compare July 12, 2020 20:00

Be-ing marked this pull request as ready for review July 12, 2020 20:00

Be-ing added 6 commits July 12, 2020 15:18

split ControllerEngine functionality into ControllerEngineJSProxy

fed2e1b

reorganize controller scripting code

9c04291

* Move src/controllers/engine to src/controllers/scripting * Create src/controllers/scripting/legacy subfolder * Rename ControllerEngine to ControllerScriptHandler * Rename ControllerEngineJSProxy to ControllerScriptInterface

controllers: clean up some #includes

24408ab

split ControllerScriptHandler into base and subclasses

979a243

This allows for a clean separation between new and legacy code.

remove hacks for JS modules in legacy controller system

b3d6ff8

ControllerScriptEngineBase: remove outdated comment

72e849f

Be-ing force-pushed the controllerengine_refactoring branch from 5eb3e4f to 72e849f Compare July 12, 2020 20:18

daschuer requested changes Jul 12, 2020

View reviewed changes

Holzhaus reviewed Jul 12, 2020

View reviewed changes

src/controllers/controller.cpp Outdated Show resolved Hide resolved

src/controllers/controller.cpp Outdated Show resolved Hide resolved

src/controllers/controller.cpp Outdated Show resolved Hide resolved

src/controllers/controller.cpp Outdated Show resolved Hide resolved

Be-ing added 4 commits July 12, 2020 17:16

Controller: replace legacy NULL with nullptr

0968a9d

ControllerScriptModuleEngine: keep build working with Qt < 5.12

77ca43f

cleanup handling of incoming HID/USB Bulk/MIDI sysex data

8153b3f

MidiController: rename confusingly overloaded "receive" method

9363b49

Be-ing force-pushed the controllerengine_refactoring branch 2 times, most recently from f6adbc9 to 9363b49 Compare July 13, 2020 17:55

ControllerScriptEngine: generate wrappers for input callbacks

e25bf12

This avoids calling two separate JS functions (one to convert the ArrayBuffer to a Uint8Array then the callback) every time controller input is received.

Be-ing force-pushed the controllerengine_refactoring branch from 3149f0e to b5cf59d Compare December 1, 2020 00:54

Be-ing mentioned this pull request Dec 2, 2020

Replace Controller "Preset" with "Mapping" mixxxdj/manual#314

Merged

Merge remote-tracking branch 'upstream/main' into controllerengine_re…

1911580

…factoring

Be-ing force-pushed the controllerengine_refactoring branch from 96aa7c7 to 1911580 Compare December 8, 2020 07:22

daschuer requested changes Dec 9, 2020

View reviewed changes

Be-ing requested a review from Holzhaus December 11, 2020 05:29

Holzhaus reviewed Dec 11, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/main' into controllerengine_re…

b5fafcc

…factoring

Be-ing closed this Dec 11, 2020

Holzhaus mentioned this pull request Dec 17, 2020

ControllerEngine Refactoring #3463

Merged

Be-ing mentioned this pull request Jul 6, 2021

Fix CoreServices initalisation order #4058

Merged

Be-ing mentioned this pull request Oct 30, 2021

Reintroduce the Quick effect selectors in the EQ preferences #4486

Merged

split up ControllerEngine #2920

split up ControllerEngine #2920

Conversation

Be-ing commented Jul 6, 2020 • edited Loading

uklotzde commented Jul 6, 2020

Be-ing commented Jul 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing Jul 12, 2020 • edited Loading

Choose a reason for hiding this comment

Be-ing Jul 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Dec 1, 2020

Be-ing commented Dec 8, 2020

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninomp commented Dec 9, 2020

Holzhaus left a comment

Choose a reason for hiding this comment

Be-ing commented Dec 11, 2020

Be-ing commented Dec 11, 2020

uklotzde commented Dec 11, 2020

Be-ing commented Dec 11, 2020 • edited Loading

Be-ing commented Dec 11, 2020

daschuer commented Dec 11, 2020

daschuer commented Dec 11, 2020

daschuer commented Dec 11, 2020

Be-ing commented Dec 11, 2020

uklotzde commented Dec 11, 2020

Be-ing commented Dec 11, 2020

Be-ing commented Dec 11, 2020

Be-ing commented Dec 15, 2020 • edited Loading

uklotzde commented Dec 15, 2020

Be-ing commented Jul 6, 2020 •

edited

Loading

Be-ing Jul 12, 2020 •

edited

Loading

Be-ing Jul 12, 2020 •

edited

Loading

Be-ing commented Dec 11, 2020 •

edited

Loading

Be-ing commented Dec 15, 2020 •

edited

Loading