Updatethrottle singleton

[rjwills28]

A little while we were looking at the performance of Phoebus when there are multiple windows open (see issue #3169), specifically that CPU usage was very high with multiple windows with PVs updating at 10Hz.

After some further investigations, including testing a standalone JavaFX application, we found that a lot of the bad performance stems from JavaFX itself and the way it handles repainting multiple windows (as also suggested by Kay in the above issue).

However, we continued to look for any ways that we could improve performance on the Phoebus side, which we address in this PR. Currently Phoebus creates an RepresentationUpdateThrottle instance for each window. This class is in charge of collecting updates and running them on the UI thread. This means that in the case of 10 windows, there are 10 RepresentationUpdateThrottle all scheduling jobs on the single UI thread.

Instead we found that it was slightly more efficient to turn the RepresentationUpdateThrottle class into a singleton and only have this one thread to collect updates across all windows and hence there is only one thread trying to run jobs on the UI thread. In testing, with this change, we saw CPU usage drop by about 10%, e.g. from 165% to 155%.

There is only one UI thread that can repaint the display and so I can't see any ill effects of only having one thread to collect updates and call this UI thread, but I would appreciate some further thoughts on this. Is there anything we haven't thought of that this might affect?

Checklist

• Testing:

  • The feature has automated tests
  • Tests were run
  • If not, explain how you tested your changes

• Documentation:

  • The feature is documented
  • The documentation is up to date
  • Release notes:
    • Added an entry if the change is breaking or significant
    • Added an entry when adding a new feature

[georgweiss]

@rjwills28 , this looks like a nice optimization.

Please check warnings issued by SonarCloud.

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[rjwills28]

Thanks for taking a look @georgweiss . I will fix the SonarQube warning before this is ready.

I came across an issue when testing which I have tried to fix but I could do with some input on the design pattern here. What I had missed was that there is a shutdown() method that is called when a window/tab is closed. Because I hadn't modified this, if you shut a window it killed the update thread and none of the other displays or a new display could get update.

There are 2 ways of fixing this:

1. Keep a reference count on the number of windows/tabs using the RepresentationUpdateThrottle and when this drops to 0, then we know it is not being used and we can perform the shutdown. This is what I have tried in this PR but there is the issue that I am setting the instance = null during shutdown, which is a static variable called in a non-static method. But we would need to set it to null so that when a display is next opened, a new RepresentationUpdateThrottle can start. This means that this class is no longer a singleton as such.
2. Remove the shutdown() entirely so that once the RepresentationUpdateThrottle has been created (i.e. when you open your first BOB screen) it is only destroyed when you shut down Phoebus. The only downside to this is that if you close all your BOB screens and then use some of the other phoebus applications (e.g. databrowser) that do not use the RepresentationUpdateThrottle, then you have this resource still running when it doesn't need to be. Although I guess it will not be doing anything as there are no displays to update.

Any input would be much appreciated!

[kasemir]

I suggest you keep the reference count and at the same time keep the one instance.
So new displays increment the reference count, shutdown will decrement, but keep the instance set to the one and only. The reference count becomes a debug tool, it's not used to release the instance, but might be useful to debug overall display startup and shutdown. Yes, this means that the one and only RepresentationUpdateThrottle will remain alive after the last display closes, but that's OK since it's not doing anything. It will be ready when the next display is opened.