ENH: Make deferring connections the default for main window again for better performance #974
Conversation
…er experience when first loading a PyDM main window
Codecov Report
📣 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
I also think the connection queuing removal was done in error in the PR you linked. I didn't catch it then during review, oops...
I like and agree with the changes in general 👍
It may be worthwhile spending a bit more time really solidifying the functionality while you're touching these areas of the code, though
(And apologies if any of this misses the mark - I'm running on only a few hours of sleep today 💤 )
pydm/main_window.py
Outdated
| @@ -370,6 +372,10 @@ def open(self, filename, macros=None, args=None, target=None): | |||
| self.ui.actionEdit_in_Designer.setText(edit_in_text) | |||
| if (self.designer_path and ui_file) or (py_file and not ui_file): | |||
| self.ui.actionEdit_in_Designer.setEnabled(True) | |||
|
|
|||
| # Create and run the thread for establishing the channel connections which have been queued above | |||
| establish_connections_thread = threading.Thread(target=data_plugins.establish_queued_connections, daemon=True) | |||
There was a problem hiding this comment.
A few thoughts here:
If kept as-is:
- The thread object should probably be retained in the main window instance (
self._establish_connections_thread?) - You might consider a private method on main window such that you can perform things before/after the
data_plugins.establish_queued_connections()call - such as adding logging around it, or cleanup in the case of an exception, etc
| macros: Optional[Dict[str, str]] = None, | ||
| args: Optional[List[str]] = None, | ||
| target: ScreenTarget = ScreenTarget.NEW_PROCESS, | ||
| defer_connections: bool = False): |
There was a problem hiding this comment.
This is a sensible default, retaining previous behavior 👍
| @@ -41,7 +53,11 @@ def connection_queue(defer_connections=False): | |||
| establish_queued_connections() | |||
|
|
|||
|
|
|||
| def establish_queued_connections(): | |||
| def establish_queued_connections() -> None: | |||
There was a problem hiding this comment.
These functions that intend to act like queue handlers and modify global state concern me a bit.
Two threads that establish queued connections have the potential to fight each other in unexpected ways (unless you introduce locks, which is less than ideal too).
It might be wise to switch these to real queues
pydm/main_window.py
Outdated
|
|
||
| # Create and run the thread for establishing the channel connections which have been queued above | ||
| establish_connections_thread = threading.Thread(target=data_plugins.establish_queued_connections, daemon=True) | ||
| establish_connections_thread.start() |
There was a problem hiding this comment.
A second thought, unrelated to the first:
We don't use the main window in our typhos stuff but may still want to benefit from the change.
- Is there another spot that PyDM could place this establish connection logic?
- Could there be a function that displays (?) call on initialization (or elsewhere) effectively saying "start a connection queue thread in the background if there isn't one already running"
- Regardless of whether the function described above is used on init, I think it would be a general purpose function that should be implemented and used in the main window
|
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 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 👍. 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 |
|
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. |
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
After
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.