perf(audeze): answer every info capability from one status read - #550
perf(audeze): answer every info capability from one status read#550AdrianKuriata wants to merge 3 commits into
Conversation
getDeviceStatus() returns battery, chatmix, sidetone and eq together, but getBattery() and getChatmix() each called it. On a Maxwell 2 that made -b -m -o json take 2.83 s instead of 1.41 s for one set of values. Cached for 500 ms, keyed on the handle since one instance serves every attached device, and mutex-guarded since it is shared.
6c494f7 to
f77991f
Compare
|
This one is clearly worth it, thanks. Setting read_at_ after the read instead of before is the right call. Please drop the mutex. HeadsetControl is single threaded, there is no other lock in the whole codebase and I would rather not start here. One thing I want handled though: the cache only expires by time, never when something is written. Right now only getBattery and getChatmix go through it and neither is affected by the setters, so nothing is broken today. But MaxwellStatus also carries sidetone, equalizer and noise filter, and setSidetone, setEqualizerPreset and setNoiseFilter all change exactly those. The moment someone adds a getter for one of them on top of statusFor, a set followed by a get within 500ms silently returns the old value and it will look like a firmware quirk, not like a cache. Either clear last_status_ in those setters or put a clear warning on statusFor. Small thing, the doc comment sits above STATUS_REUSE_WINDOW so it reads as if it documents the constant. Move it down to statusFor. I have no Maxwell 2 here, so I only verified that it builds and the tests pass and took your measurements as given. For the intermittent status reads please open a separate issue, otherwise it gets lost in here. |
Review feedback: the codebase is single threaded and has no other lock, so the mutex goes. The reused status also carries sidetone, equalizer and noise filter, which setSidetone, setEqualizerPreset and setNoiseFilter all write. Nothing reads those through statusFor today, but a getter added later would see the value from before the write, so the setters now drop the cached status. Moved the doc comment onto statusFor, where it belongs, instead of above the window constant.
|
Done in 293db49. Mutex is gone, along with the On the cache never expiring on write: I went with clearing rather than warning, since a warning only helps whoever reads it. There is now an The doc comment moved onto
I have not opened the issue for the intermittent status reads yet, so that one is still outstanding on my side rather than done. |
cpp-linter checks whole files, not changed lines, and the trailing comment on the Xbox product id predates this PR. It is not reachable from master's own CI, which only runs clang-format on pull requests, so it went unnoticed. clang-format 18.1.3 output, nothing hand-written.
|
Same note as on #549: I did get clang-format 18.1.3 in the end, so formatting is checked rather than guessed, and cpp-linter passes. Commit b5b2c59 is that check. The workflow runs with |
b5b2c59 to
6c009d0
Compare
2 participants
getDeviceStatus() sends 15 init packets plus 6 status requests, each after a 60 ms
delay, and returns battery, chatmix, sidetone, equalizer and noise filter
together. But getBattery() and getChatmix() each call it, so asking for both pays
twice for one set of numbers.
Audeze Maxwell 2 (0x3329:0x4b28), release build:
With the status reused for 500 ms the second one drops to 1.41 s. The window is
much shorter than the 1.4 s read it saves, so nothing gets a value staler than its
own read would have produced. Reading both from one pass also makes them a
coherent snapshot instead of two readings 1.4 s apart.
Keyed on the device handle since DeviceRegistry keeps one instance per device
class, and mutex-guarded since that instance is shared - it is the only mutable
state in the class. Happy to drop the lock if you consider concurrent calls out of
scope.
headsetcontrol_tests passes.
Unrelated but visible if you test this: status reads on this device fail
intermittently on master too - five consecutive -b -m -o json runs gave
battery = -1, 74, -1, -1, 74 - so a flaky reading is not a regression from this
patch.
Independent of #549, different file and different cause; either can go in without
the other.