[emilioheredia-source]

Target branch: ControlSystemStudio/phoebus:master
Source branch: emilioheredia-source/phoebus:display/serialize-display-loads
Depends on: nothing

---

Summary

DisplayRuntimeInstance.loadDisplayFile() disposes the display that is shown, then loads the new one in a background job. A load that is still in its job is not tracked anywhere until its display is represented. When a second load starts in that time, nothing disposes the first one, and its job still represents the display and starts its runtime. Two things go wrong:

• The runtime of the skipped display keeps running, with its PVs connected and its scripts and rules active, until Phoebus exits.
• Whichever load finishes last is shown, so the window can show a different display than the navigation history says.

Closing the display while a load is running has a related problem: the load continues, and a "Cannot load model" error appears after the display is gone, together with an IllegalArgumentException: Expecting Group or Pane, got null on the UI thread.

A double click on the toolbar's "back" button is enough to hit it. Large displays, which take longer to load, make it more likely. With a display of about 300 widgets, two clicks 30 ms apart left all 21 PVs of the skipped display connected (checked in the PV pool's log).

With this PR, loads of a display instance run one at a time, up to the start of their runtime:

• The shown display remains until the new one has been loaded. Then a single step on the UI thread disposes it and represents the new one, so the previous display is only disposed once its own load has started its runtime. Before, the dispose ran right away when the load was requested, and a "Loading ..." message was shown until the new display was represented. The message is still shown for the first display of a tab. Waiting for embedded displays to be represented happens after the runtime has started, so it doesn't hold up the next load.
• A load that was replaced by a newer one, or by closing the display, is skipped. If its model was already loaded, it is disposed without being represented or started.
• An error of a replaced load is only logged. It no longer shows an error dialog or closes the display that the newer load is using.

Restoring 15 example displays in 3 windows at startup takes the same time as on master (see the comment below for the numbers).

To keep loadDisplayFile() readable, the body of the background job is split into a few helpers. Apart from the dispose step and the checks above, the code is unchanged; git diff -w shows the actual change.

Not covered here: the "replace" target of an Open Display action doesn't go through loadDisplayFile(). It replaces the representation directly on the UI thread.

---

Testing

1. Display with history A, B, C. Double click "back": A is shown, and only A's PVs are open. On master, B's PVs stay open, or B is shown.
2. Normal navigation, reload (F5), opening a display that fails to load (error dialog, tab closes): unchanged.
3. Close a display while it loads: no error (checked by the test, which closes the display before the load can start).

Checklist

• Testing:
  • The feature has automated tests (DisplayLoadSequenceUI: two loads in a row, a replaced load that fails, closing a display while it loads; all three fail on master)
  • Tests were run (display runtime module tests on JDK 25; the TestFX test with -Dtestfx.robot=glass under Xvfb)

The TestFX test follows the *UI naming of the ui-tests profile. That profile currently runs no tests: failsafe 2.22.0 fails to discover JUnit 5 tests with the current JUnit platform (NoSuchMethodError ... getDiscoveryListener()), also for the existing UI tests in core/ui. So for now the test was run with surefire, as above.

[georgweiss]

@emilioheredia-source , this looks very interesting. Would you mind testing a case where a large number (>10) of non-trivial displays are open, maybe across multiple windows? The test should just shut down Phoebus and restart to recreate the displays as they were when shutting down (default Phoebus behavior).

Reason I ask is to figure out how user experience would be affected, e.g. seemingly longer time to restore the view when launching Phoebus.

[emilioheredia-source]

Thanks, that was a good question. The test found a real slowdown, which is fixed now.

Setup: 15 displays from the examples folder (10_scripts, 01_main, 11_datasources, 12_formulas, plots_xy, plots_image, structure_tabs, monitors_table, monitors_symbols, ...), 5 tabs in each of 3 windows, saved as the memento. Phoebus is started and restores them. I measured the time from launching Phoebus until the last display logged "Done with representing model", alternating master and the PR, on Xvfb with software rendering.

	All 15 displays represented	First display
master (9 runs)	3.12 – 3.20 s, one run 3.90 s	about 2.7 s
PR as first pushed (2 runs)	3.60 – 3.77 s	about 3.2 s
PR now (4 runs)	3.13 – 3.23 s	about 2.7 s

The first version was about 0.4 s slower. Each load went to the UI thread twice: once to dispose the shown display before loading, then again to represent the new one. At startup, the first layout pass of the restored windows ran in between, so every display waited for it.

The new commit disposes the shown display and represents the new one in a single UI step, after the new display has been loaded, as master does for the representation. With that, restoring takes the same time as on master. One visible difference when navigating: the shown display now remains until the new one is ready, instead of being replaced by a "Loading ..." message right away. The first display of a tab still shows the message.

In case it helps to judge the impact: we came across the leak while adding the mouse back/forward buttons for our operators, who navigate regularly with Back and Forward in EDM-style windows. A double click on the toolbar's Back button is enough to leave the skipped display's PVs connected until Phoebus exits.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
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

[shroffk]

Thanks @emilioheredia-source
This is an important leak you have discovered and thanks for the insight into the problem.

I believe we have moved the file loading off the UI thread due to various performance issues discovered in BOY. This is particularly true if you have a bad script...

That having being said the resource leak you point out is a serious bug.

then loads the new one in a background job. A load that is still in its job is not tracked anywhere until its display is represented. When a second load starts in that time, nothing disposes the first one, and its job still represents the display and starts its runtime.

Based on your finding the key issue here is that the job is not tracked properly. Instead of getting rid of the background loading could be make it into a proper cancellable job ( using CompletableFuture maybe? ) and a proper tracking of it. This would address the leak while preserving the UX ( visual indication that new file is being loading by removing the old one and being more performant in situation where multiple files are being loaded )

I remember with BOY when the existing screen stayed visisble... instead of waiting users were inclined to keep pressing the button more and more - out of frustration - making matters worse.

Again, we are quite glad you have discovered this