From 577f5501a698933e402cbe3c65f7b8ba8ee1bcae Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:42:49 -0400 Subject: [PATCH] perf(plugins): stop re-deriving a display() signature the caller already cached (#549) display_controller resolves once, and caches, whether a plugin's display() takes a display_mode keyword -- self._plugin_accepts_display_mode, populated right before the dispatch. It then handed the executor a types.SimpleNamespace wrapping a closure, and execute_display() ran inspect.signature() on that to work out the same thing. Because the SimpleNamespace is rebuilt per call, the callable was new every time, so nothing inside the executor could ever cache it either. Measured at ~39us per dispatch on a Pi 4, for a value the caller had a line earlier. execute_display() now takes accepts_display_mode, falling back to inspecting only when a caller does not pass it, so existing callers are unaffected. Also documents two things that read as bugs and are not: - execute_with_timeout()'s timeout is advisory. Nothing cancels the thread -- Python cannot -- so on expiry the operation runs to completion in the background and only the caller gives up. A permanently hung plugin leaks a daemon thread per attempt. This is why callers holding a lock across the call must release it from inside the wrapped callable, as run()'s _release_display_lock already does. - Only the first display() of each mode goes through the executor; the per-frame loops call display() directly. That is deliberate: a thread per frame would cost more than an advisory timeout buys. Both loops now say so, so the asymmetry does not read as an oversight. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) --- src/display_controller.py | 27 +++++++++++++++++++++++- src/plugin_system/plugin_executor.py | 31 +++++++++++++++++++++++----- 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/src/display_controller.py b/src/display_controller.py index 8a902570..44875f63 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -2169,7 +2169,14 @@ class DisplayController: types.SimpleNamespace(display=_display_target), plugin_id, force_clear=self.force_change, - display_mode=active_mode if _accepts_display_mode else None + display_mode=active_mode if _accepts_display_mode else None, + # Already resolved and cached above. + # Without this the executor re-derives + # it with inspect.signature() against + # the SimpleNamespace built two lines + # up -- a fresh callable every call, so + # nothing there can ever cache. + accepts_display_mode=_accepts_display_mode ) except Exception: # pragma: no cover - defensive; # execute_display catches everything @@ -2492,6 +2499,15 @@ class DisplayController: 1.0 / display_interval ) + # Deliberate: frames after the first call + # display() directly rather than through + # PluginExecutor. The executor spawns a thread per + # call, which at this loop's frame rate would cost + # more than the advisory timeout it buys -- and + # that timeout cannot cancel a hung plugin anyway + # (see execute_with_timeout). The first dispatch + # above still goes through it, so load-time + # failures are still caught and recorded. while True: _frame_start = time.perf_counter() try: @@ -2564,6 +2580,15 @@ class DisplayController: display_interval ) + # Deliberate: frames after the first call + # display() directly rather than through + # PluginExecutor. The executor spawns a thread per + # call, which at this loop's frame rate would cost + # more than the advisory timeout it buys -- and + # that timeout cannot cancel a hung plugin anyway + # (see execute_with_timeout). The first dispatch + # above still goes through it, so load-time + # failures are still caught and recorded. while True: time.sleep(display_interval) self._tick_plugin_updates() diff --git a/src/plugin_system/plugin_executor.py b/src/plugin_system/plugin_executor.py index 2fac5c5e..c1a23b49 100644 --- a/src/plugin_system/plugin_executor.py +++ b/src/plugin_system/plugin_executor.py @@ -76,6 +76,13 @@ class PluginExecutor: thread.start() thread.join(timeout=timeout) + # NB: this timeout is advisory. Nothing cancels the thread -- Python + # has no way to -- so on expiry the operation keeps running to + # completion in the background and only this caller gives up waiting. + # A plugin that hangs permanently leaks one daemon thread per attempt. + # Callers that hold a resource across the call must release it from + # inside the wrapped callable rather than after this returns; see the + # _release_display_lock guard inside DisplayController.run(). if not result_container['completed']: error_msg = f"{plugin_context} operation timed out after {timeout}s" self.logger.error(error_msg) @@ -148,7 +155,8 @@ class PluginExecutor: plugin_id: str, force_clear: bool = False, display_mode: Optional[str] = None, - timeout: Optional[float] = None + timeout: Optional[float] = None, + accepts_display_mode: Optional[bool] = None ) -> bool: """ Execute plugin display() method with error handling. @@ -159,6 +167,9 @@ class PluginExecutor: force_clear: Whether to force clear display display_mode: Optional display mode parameter timeout: Timeout in seconds (None = use default) + accepts_display_mode: Whether plugin.display() takes a + display_mode keyword. Pass it when the caller already knows; + None falls back to inspecting the callable. Returns: True if display succeeded, False otherwise @@ -166,10 +177,20 @@ class PluginExecutor: try: start_time = time.time() - # Check if plugin accepts display_mode parameter - import inspect - sig = inspect.signature(plugin.display) - has_display_mode = 'display_mode' in sig.parameters + # Does display() take a display_mode keyword? The caller usually + # knows and caches the answer, so prefer what it passed. + # + # Inspecting here was not merely redundant, it could never be + # cached: display_controller wraps the real plugin in a fresh + # SimpleNamespace per call, so inspect.signature() saw a new + # callable every time and paid ~55us on a Pi 4 to re-derive a + # value the caller had computed one line earlier and stored in + # self._plugin_accepts_display_mode. + if accepts_display_mode is None: + import inspect + accepts_display_mode = ( + 'display_mode' in inspect.signature(plugin.display).parameters) + has_display_mode = accepts_display_mode # Capture the return value from the plugin's display() method if has_display_mode and display_mode: