perf(cli): skip the forced info reads when the invocation performs an action - #549
perf(cli): skip the forced info reads when the invocation performs an action#549AdrianKuriata wants to merge 1 commit into
Conversation
Structured output marks every info capability as requested, even when the command line only asked to set a value. On an Audeze Maxwell 2 that turns a 0.07 s write into 2.90 s, because the status read sends 21 packets at 60 ms intervals. Explicitly requested info still works, -b -s 20 -o json reports the battery.
eb86c8f to
5941ecf
Compare
|
Thanks, the numbers speak for themselves here. I agree the old behaviour is the bug, so no flag please, keep it as the default. Two things before this can go in. The output shape changes, so please bump API_VERSION in cli/output/output.cpp to 1.5. Thats what it is there for, otherwise a consumer has no way to notice. And enableExtendedInfoRequests runs before handleMultiDeviceActions, so hasRequestedAction looks at action requests that get neutralized one line later. It also checks all devices at once, so one device with an action switches off the info reads of another device that never had one. Please move the call after handleMultiDeviceActions and do the check per device instead of globally. Also worth writing into the PR text: it does not only drop info when you set something, it drops everything you did not explicitly ask for. With -b -s 20 -o json you now get battery but no chatmix, master returned both. Fine by me, but people will notice. Not caused by you, I just saw it while reading this: the multiple devices guard in handleMultiDeviceActions does not actually do anything. It sets result.message and value but leaves should_process true, and status was already NotProcessed, so processFeatureRequests runs the action anyway. I will fix that separately. It is also why the ordering above matters, once that guard works your version would return neither the action nor the info. |
Structured output (json/yaml/env) marks every info capability as requested, no
matter what the command line actually asked for. On a device with an expensive
status read that dominates an action which is otherwise instant.
Audeze Maxwell 2 (0x3329:0x4b28), release build:
The difference is the status read - getDeviceStatus() sends 15 init packets plus
6 status requests, each after a 60 ms delay. The sidetone write itself is one
packet. Anything driving the CLI per user interaction pays that every time; a GUI
moving a slider ends up queueing writes for minutes.
After the patch the same call takes 0.07 s and the actions array is unchanged. A
pure query is unaffected: --output json still takes 2.83 s and still reports
battery and chatmix. Info asked for explicitly still works, -b -s 20 -o json
reports the battery in 1.49 s.
headsetcontrol_tests passes.
One thing to decide: this changes the json shape for action invocations,
devices[].battery is no longer there when you only asked to set something. I think
the old behaviour is the bug, but if you would rather keep the default I can put
it behind a flag instead.