pydm icon indicating copy to clipboard operation
pydm copied to clipboard

ENH: Make deferring connections the default for main window again for better performance

Open jbellister-slac opened this issue 2 years ago • 7 comments

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.

jbellister-slac avatar Feb 22 '23 01:02 jbellister-slac

Codecov Report

Merging #974 (1ca0f77) into master (6b39167) will increase coverage by 0.01%. The diff coverage is 81.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.

codecov-commenter avatar Feb 22 '23 02:02 codecov-commenter

That all makes sense to me, thank you!

jbellister-slac avatar Feb 22 '23 20:02 jbellister-slac

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.

ZLLentz avatar Feb 22 '23 20:02 ZLLentz

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.

ZLLentz avatar Feb 22 '23 20:02 ZLLentz

Thanks for pointing that out. I will take a look and see what is going wrong in that case.

jbellister-slac avatar Feb 22 '23 20:02 jbellister-slac

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.

ZLLentz avatar Feb 22 '23 21:02 ZLLentz

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.

jbellister-slac avatar Mar 31 '23 19:03 jbellister-slac