pydm
pydm copied to clipboard
ENH: Make deferring connections the default for main window again for better performance
Context
As mentioned in the previous performance PR, load time of establishing connections was what I wanted to take a look at next as a lot of time is spent making connections to channels upon starting up a display. While looking through the relevant code, I found that there is already a mechanism for making this better, a connection queue that can be appended to and then processed at a later time. I did some digging to find out why this wasn't being used much just in case there was an issue with it, but I think its use may have been removed inadvertently?:
It was added here: #467 Deferring connections was made the default here: #489 And then its use was removed here, maybe just accidentally dropped among all the changes as there's no mention of why it was removed: #590
What's Changing
This makes using the connection queue/deferring the default again for the main window. By doing it this way, displays with hundreds or thousands of connections appear to be much more responsive as the display will appear sooner and be responsive to user input faster as it is no longer required to establish every single connection prior to becoming responsive.
I've also removed the use of deferring connections from the template repeater, this appeared to be slowing down performance in most test cases as the resulting call to establish the connections was not being delayed enough to make any big difference.
This seems to speed up response times in a couple large displays I tested by anywhere from ~15% - 40% depending on number of connections made at startup.
Before
Total time: 10.2724 s
352 2 11413.0 5706.5 88.8 new_widget = load_file(filename,
353 1 0.0 0.0 0.0 macros=macros,
354 1 0.0 0.0 0.0 args=args,
355 1 0.0 0.0 0.0 target=target)
After
Total time: 6.87503 s
353 2 5365.4 2682.7 79.2 new_widget = load_file(filename,
354 1 0.0 0.0 0.0 macros=macros,
355 1 0.0 0.0 0.0 args=args,
356 1 0.0 0.0 0.0 target=target,
357 1 0.0 0.0 0.0 defer_connections=True)
I also compared this defer strategy to just making connections in background threads, but the threaded approach did not help nearly as much as it is just taking time from the main thread during display initialization.
Testing
Verified performance increases on several large displays on the accelerator side including lclshome. Tried pmps and saw speedups although again without any actual connections.
Added a simple test case for verifying setting of the parameter.
Codecov Report
Merging #974 (1ca0f77) into master (6b39167) will increase coverage by
0.01%
. The diff coverage is81.81%
.
:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
@@ Coverage Diff @@
## master #974 +/- ##
==========================================
+ Coverage 59.79% 59.80% +0.01%
==========================================
Files 104 104
Lines 13666 13669 +3
==========================================
+ Hits 8171 8175 +4
+ Misses 5495 5494 -1
Impacted Files | Coverage Δ | |
---|---|---|
pydm/widgets/template_repeater.py | 48.62% <66.66%> (-0.40%) |
:arrow_down: |
pydm/data_plugins/__init__.py | 89.70% <100.00%> (+0.73%) |
:arrow_up: |
pydm/display.py | 77.17% <100.00%> (+0.25%) |
:arrow_up: |
pydm/main_window.py | 49.18% <100.00%> (+0.35%) |
:arrow_up: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
That all makes sense to me, thank you!
I don't have suggestions about the code beyond Ken's suggestions, but I ran this with the pmps-ui
with live PVs.
This causes the screen to open and be ready for user input in ~10-15 seconds, and then it slowly fills in all the PVs properly. Working as intended with great results :+1:. Previously, user input was delayed up to 30+ seconds.
It breaks my filtering on one of the tabs- my gui is convinced that all the widgets on that tab are staying disconnected, despite showing values, so it wants to hide them all by default. I'll check a bit to see what the discrepancy is.
As far as I can tell, any PyDMChannel
objects I create and call connect
on in code prior to the queue being emptied never actually get connected. I don't understand why this happening after some looking through both the pydm source and the pmps-ui source.
Thanks for pointing that out. I will take a look and see what is going wrong in that case.
It's really confusing to me since all of the widgets load fine, it's just the channels I create in the background that fail. Most of these re-use channel addresses that are also used in the widgets, which connect normally.
I have not forgotten about this one, just pushing it off to the next release for now. The issue with the tab in the pmps display not loading properly turned out to be the use of native python threading as opposed to the pyqt version. I did not go deep into the pyqt code to figure out why yet, but it's likely signal/slot connections not working properly. Switching over to a pyqt thread makes it work fine.
I still did want to take another pass at improving performance though as well as testing thoroughly again before putting this back up for review.