Added Trace for the mapping connections, to allow JS profiling. #4766
Conversation
There was a problem hiding this comment.
This will allocate a new string on each CO callback call, which is inefficient on its own. Moreover, you can already profile javascript in 2.4 via Qt's built-in console api (console.profile). https://doc.qt.io/qtcreator/creator-qml-performance-monitor.html
|
The console.profile command requires an external attached profiler tool. AFAIK, this is only supported by the QML engine, not by QJSEngine used for controller mappings. But even than, it would require a lot more skills from mapping developers, than the build in Stats window, which just needs Mixxx to be called in developer mode. |
I'm not sure either, but if its only part of the QMLEngine and not QJSEngine, why would they make it part of the API offered by the QJSEngine? There are other QML-only APIs.
The majority of mapping developers won't have to concern themselves with optimizing their mappings. Optimizing Javascript is a very tricky task in itself because of the nature of the language and runtime. Simply providing a counter is likely not adequate. You only count the CO that is called which could be hooked up to many different callbacks, so you don't know which callback is actually the problem. Besides, the current implementation will very likely cause major performance penalties in a hot path of the code. So unless you can minimize that, I don't want to merge this. |
| std::unique_ptr<Trace> pCallCallbackTrace; | ||
| pCallCallbackTrace = std::make_unique<Trace>( | ||
| QString("JS " + key.item + " callback").toStdString().c_str()); |
There was a problem hiding this comment.
why the unique_ptr? The trace object is not passed around, it could easily be allocated on the stack.
The creation of the QString also involves a heap allocation (likely since the string is likely quite long). toStdString might involves a heap allocation or at least a copy.
There was a problem hiding this comment.
The trace Object id designed to live on the stack and will have almost zero overhead in normal mode.
You must not use QString here, because is also allocates memory in non developer mode.
This is the correct usage for minimum over-head at non developer mode:
mixxx/src/soundio/sounddevicenetwork.cpp
Line 415 in d5cf277
To speed up the debug case as well, you may consider to introduce a char16_t* overload of Trace and pass the string as u" "
Or even better a template taking char16_t[] to make use of sizeoff() instead of parsing for "/0"
| std::unique_ptr<Trace> pCallCallbackTrace; | ||
| pCallCallbackTrace = std::make_unique<Trace>( | ||
| QString("JS " + key.item + " callback").toStdString().c_str()); |
There was a problem hiding this comment.
The trace Object id designed to live on the stack and will have almost zero overhead in normal mode.
You must not use QString here, because is also allocates memory in non developer mode.
This is the correct usage for minimum over-head at non developer mode:
mixxx/src/soundio/sounddevicenetwork.cpp
Line 415 in d5cf277
To speed up the debug case as well, you may consider to introduce a char16_t* overload of Trace and pass the string as u" "
Or even better a template taking char16_t[] to make use of sizeoff() instead of parsing for "/0"
JoergAtGithub
force-pushed
the
TraceJavascriptConnection
branch
from
6364b59
to
bbc2dba
Compare
…ct of Trace in normal mode
To use char16_t* requires to use https://doc.qt.io/qt-5/qstring.html#fromUtf16-1 where the documentation says: "This function is slow compared to the other Unicode conversions." |
|
Thanks the current solution eliminates my efficiency concerns, but I'm still not convinced that this is particularly useful. Optimizing a function when you only know its total runtime is usually just stumbling around in the dark. You need a more detailed trace of the contained functions as well which is usually collected by a profiler (such as the ones included its Qt's debugging tools). |
Did a little bit of research, this is only implicitly the case. See: https://bugreports.qt.io/browse/QTBUG-65419 I would consider merging this as a stopgap solution, though my primary concern still is, that constructing the tag based on the CO key does not really help when trying to find what callback exactly is making problems. |
|
Sure... |
This PR adds a Trace statement for each connection
It's grouped by control name - not by group+name - to keep the number of entries limited. I expected that the controls show the same performance for all groups/channels.
How to test:
mixxx --developer
Menu -> Developer -> Developer Tools
Under Stats you should see two lines per JS control connection to your mapping - as for any other trace.
PS: You might notice the huge times in my screenshot - this is a correct measurement, caused by inefficient JavaScript code in common-hid-packet-parser.js (HIDController.prototype.getOutputField)