replace timers with functions [CPP-872] #972
Conversation
adrian-kong
changed the title
adrian-kong
changed the title
| running: true | ||
| repeat: true | ||
| onTriggered: { | ||
| if (!advancedImuTab.visible) |
There was a problem hiding this comment.
can remove .visible checks since capnproto is paused in backend when not in view.
the capnproto pause however isnt fully pause i.e. will still update anything in advanced subtabs
might be cleaner just to move to entirely pausing on capnproto side.
|
Frontend and Release Workflow Started here |
There was a problem hiding this comment.
In general, I think this is a great idea and we should take this update.
However I have two main concerns:
-
There is no explicit rate-limiting of calling the update function. I guess it probably is fine because the rust backend is rate limited at some level, but I'm not sure if we want to explicitly do something here at the front end, otherwise there could be perf issues.
-
I haven't carefully looked at all of the code you refactored, but the timer would be called regardless if the data are updated. It's possible that people could have snuck code in there that may have handled things like window/zoom resizing or something that only had to do with frontend changes. Did you carefully audit the timer codes to confirm this wasn't the case? or if there were such things, they should be attached to the slot where the signal for that change would be emitted.
not too sure on this but im guessing they have their own FPS cap for repainting
oh yea good point. will look into this some more. |
|
@adrian-kong yeah for #2 i didn't check closely enough to see if there were particular problems that would show up, everything i saw looked ok, i just wanted to bring it up in case you hadn't checked. As for the limit on repainting, i think you are correct. But there could still be other expensive stuff that slows it down and affect perf. Just something to think about as you are giving the code a once-over. These were just concerns i had, i don't think they are necessarily blockers to taking this PR. |
| property bool labelsVisible: labelsVisibleCheckBox.checked | ||
| property bool polarChartWidthChanging: false | ||
|
|
||
| function clearLabels() { |
There was a problem hiding this comment.
@pcrumley yeah it actually introduced a bug (no longer getting as frequent repainting of labels) so the timer sleep was making the labels flash.
this should be fixed now, also less refreshing now
|
Frontend and Release Workflow Started here |
There was a problem hiding this comment.
How will this affect things like the refresh rate switch? How well does this run on the surface tablet that you have for testing?
swift-toolbox/swiftnav_console/main.py
Line 721 in 6acfd17
|
Showing a pretty big jump in CPU usage on my laptop. Old New |
are you in the Maps tab or have clicked into it to initialize in both instance? seems like QtWeebEngineProcess is taking up most the CPU (also good thing about pyside is apparently is if you dont click into the map, the js wont load so thats nice but qtwebengineprocess would run in background by default since webview component declared explicitly in the qml)
played around refresh rate switch doesnt do much after these changes but this does enforce repainting on data tho which is pretty nice, would think theres a way to cap FPS on pyside instead maybe?
haven't tested on windows but for mac, the settings tab is much better where previously i was unable to scroll on |
Yes, because the refresh rate switch is made useless by this PR -- we should remove it or rethink how it works. The way this worked in the previous console was to just record the last update time and skip an update if it was "too soon" -- so, effectively let the data stream drive updates, but painting would be skipped if it was exceeding the configured refresh rate.
Maybe we should think about making a more targeted update(s) first then, like just removing the timer from the settings view? |
|
More informal benchmarks -- Latest main branch: + have not clicked into the map tab + tracking view full ==>
This branch: + have not clicked into the map tab + tracking view full ==>
After running for a while the main branch does start to look more like this branch, so not sure there's a clear perf story here, but definitely seems like the web process is doing a lot more than it should be (especially if it's not loaded):
|
|
Frontend and Release Workflow Started here |
|
removes refresh rate, since this makes the option redundant. |
|
Frontend and Release Workflow Started here |
|
@silverjam this seems to have roughly identical ~30% cpu (+- 1%) performance for cpu utilization after #989 |
|
@pcrumley Please approved if your concerns have been addressed. |
|
Frontend and Release Workflow Started here |
4 participants
replaces timers to update whenever received new data