Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
20 changes: 18 additions & 2 deletions pydm/data_plugins/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,19 @@


@contextmanager
def connection_queue(defer_connections=False):
def connection_queue(defer_connections: bool = False) -> None:
"""
Creates a queue for holding channel connections and potentially processing them at a later time. When
defer_connections is set to True, the exit from the context manager will not result in any connections being made.
This allows for a more responsive end user experience on displays with many connections as it will load without
needing to establish all connections first.
Parameters
----------
defer_connections : bool
Whether or not to defer making the actual connections to channels at a later time. Note that if this
is set to true, a call to establish_queued_connections() must be made at some point later as the queue
will not be automatically be processed by the context manager in this case.
"""
global __CONNECTION_QUEUE__
global __DEFER_CONNECTIONS__
if __CONNECTION_QUEUE__ is None:
Expand All @@ -41,7 +53,11 @@ def connection_queue(defer_connections=False):
establish_queued_connections()


def establish_queued_connections():
def establish_queued_connections() -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""
Processes all channels in the deferred connection queue establishing the actual connection for each. Upon
completion, will reset the global connection queue to None.
"""
global __DEFER_CONNECTIONS__
global __CONNECTION_QUEUE__
if __CONNECTION_QUEUE__ is None:
Expand Down
17 changes: 14 additions & 3 deletions pydm/display.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@
from io import StringIO
from os import path
from string import Template
from typing import Dict, Optional, Tuple
from typing import Dict, List, Optional, Tuple

import re
import six
from qtpy import uic
from qtpy.QtWidgets import QApplication, QWidget

from . import data_plugins
from .utilities import import_module_by_filename, is_pydm_app, macro


Expand All @@ -28,7 +29,11 @@ class ScreenTarget:
logger = logging.getLogger(__file__)


def load_file(file, macros=None, args=None, target=ScreenTarget.NEW_PROCESS):
def load_file(file: str,
macros: Optional[Dict[str, str]] = None,
args: Optional[List[str]] = None,
target: ScreenTarget = ScreenTarget.NEW_PROCESS,
defer_connections: bool = False):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a sensible default, retaining previous behavior 👍

"""
Load .ui, .py, or .adl screen file, perform macro substitution, then return
the resulting QWidget.
Expand All @@ -47,6 +52,11 @@ def load_file(file, macros=None, args=None, target=ScreenTarget.NEW_PROCESS):
target : int
One of the ScreenTarget targets. PROCESS is only available when used
with PyDM Application for now.
defer_connections : bool
If set to true, the loading of channel connections will be deferred until the
connection queue is flushed via a data_plugins.establish_queued_connections() call. This
results in the display being responsive to the user faster as it does not wait on
establishing connections during initial display creation.

Returns
-------
Expand All @@ -66,7 +76,8 @@ def load_file(file, macros=None, args=None, target=ScreenTarget.NEW_PROCESS):
_, extension = os.path.splitext(file)
loader = _extension_to_loader.get(extension, load_py_file)
logger.debug("Loading %s file by way of %s...", file, loader.__name__)
w = loader(file, args=args, macros=macros)
with data_plugins.connection_queue(defer_connections=defer_connections):
w = loader(file, args=args, macros=macros)
if target == ScreenTarget.DIALOG:
w.show()

Expand Down
8 changes: 7 additions & 1 deletion pydm/main_window.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from .widgets.rules import register_widget_rules, unregister_widget_rules
from . import config
import subprocess
import threading
import platform
import logging

Expand Down Expand Up @@ -352,7 +353,8 @@ def open(self, filename, macros=None, args=None, target=None):
new_widget = load_file(filename,
macros=macros,
args=args,
target=target)
target=target,
defer_connections=True)
if new_widget:
if self.home_widget is None:
self.home_widget = new_widget
Expand All @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few thoughts here:

If kept as-is:

  1. The thread object should probably be retained in the main window instance (self._establish_connections_thread?)
  2. 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

establish_connections_thread.start()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return new_widget

def load_tool(self, checked):
Expand Down
19 changes: 15 additions & 4 deletions pydm/tests/test_display.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import os
import pytest
from pydm import Display
from pydm.display import load_py_file, _compile_ui_file
from qtpy.QtWidgets import QWidget
import pydm.utilities.stylesheet
from pydm import Display, data_plugins
from pydm.display import load_file, load_py_file, _compile_ui_file, ScreenTarget

# The path to the .ui file used in these tests
test_ui_path = os.path.join(
Expand Down Expand Up @@ -114,3 +112,16 @@ def test_compile_ui_file():
assert class_name == 'Ui_Form'
assert 'setupUi(self' in code_string
assert 'retranslateUi(self' in code_string


def test_defer_connections(qtbot):
"""
Verify that when the defer_connections parameter is set to true, connections are held up in a queue
until it is time to process them
"""
load_file(valid_display_test_py_path, target=ScreenTarget.HOME, defer_connections=True)

# The test file loaded has one connection to TST:Val1. Since defer_connections is True, verify
# that this address has been placed in the queue.
assert 'TST:Val1' == data_plugins.__CONNECTION_QUEUE__[0].address
data_plugins.__CONNECTION_QUEUE__ = None
Loading