perf(bluez): serve Base property reads from the proxy cache

blueman/bluez/Base.py

@@ -10,20 +10,47 @@
 DBUS_TIMEOUT = 10 * 1_000
 
 
+class InstanceRegistry:
+    """Caches one :class:`Base` instance per D-Bus object path.
+
+    Object-identity caching used to live inline in :class:`BaseMeta`. Pulling
+    it into a small, named collaborator keeps the metaclass focused on
+    construction and makes the cache independently testable and replaceable.
+    """
+
+    def __init__(self) -> None:
+        self._instances: dict[str, "Base"] = {}
+
+    def get(self, path: str) -> "Base | None":
+        return self._instances.get(path)
+
+    def add(self, path: str, instance: "Base") -> None:
+        self._instances[path] = instance
+
+    def remove(self, path: str) -> None:
+        self._instances.pop(path, None)
+
+    def clear(self) -> None:
+        self._instances.clear()
+
+
 class BaseMeta(GObjectMeta):
     def __call__(cls, *args: object, **kwargs: str) -> "Base":
-        if not hasattr(cls, "__instances__"):
-            cls.__instances__: dict[str, "Base"] = {}
+        registry: InstanceRegistry | None = cls.__dict__.get("_registry")
+        if registry is None:
+            registry = InstanceRegistry()
+            cls._registry = registry
 
         path = kwargs.get('obj_path')
         if path is None:
             path = getattr(cls, "_obj_path")
 
-        if path in cls.__instances__:
-            return cls.__instances__[path]
+        existing = registry.get(path)
+        if existing is not None:
+            return existing
 
         instance: "Base" = super().__call__(*args, **kwargs)
-        cls.__instances__[path] = instance
+        registry.add(path, instance)
 
         return instance
 
@@ -36,7 +63,7 @@ class Base(GObject.Object, metaclass=BaseMeta):
     __gsignals__: GSignals = {
         'property-changed': (GObject.SignalFlags.NO_HOOKS, None, (str, object, str))
     }
-    __instances__: dict[str, "Base"]
+    _registry: InstanceRegistry
 
     _interface_name: str
 
@@ -62,15 +89,24 @@ def __init__(self, *, obj_path: ObjectPath):
 
         self.__variant_map = {str: 's', int: 'u', bool: 'b'}
 
+        # Properties whose last forced refresh failed and are being served from
+        # cache. PropertiesChanged clears the flag once a live value arrives.
+        self.__stale: set[str] = set()
+
     def _properties_changed(self, _proxy: Gio.DBusProxy, changed_properties: GLib.Variant,
                             invalidated_properties: list[str]) -> None:
         changed = changed_properties.unpack()
         object_path = self.get_object_path()
         logging.debug(f"{object_path} {changed} {invalidated_properties} {self}")
 
         for key in list(changed) + invalidated_properties:
+            self.__stale.discard(key)
             self.emit("property-changed", key, changed.get(key, None), object_path)
 
+    def is_stale(self, name: str) -> bool:
+        """True if ``name`` is currently served from cache after a failed refresh."""
+        return name in self.__stale
+
     def _call(
         self,
         method: str,
@@ -97,23 +133,38 @@ def callback(
         self.__proxy.call(method, param, Gio.DBusCallFlags.NONE, DBUS_TIMEOUT, None,
                           callback, reply_handler, error_handler)
 
-    def get(self, name: str) -> Any:
+    def get(self, name: str, fresh: bool = False) -> Any:
+        # Prefer the proxy's local property cache, which Gio keeps current from
+        # PropertiesChanged signals, over a synchronous Properties.Get round-trip
+        # on every read. Bluez devices expose the same properties repeatedly to
+        # the UI; serving them from cache avoids a blocking D-Bus call each time.
+        # Callers that need a guaranteed-live value pass fresh=True.
+        if not fresh:
+            cached = self.__proxy.get_cached_property(name)
+            if cached is not None:
+                return cached.unpack()
+
         try:
             prop = self.__proxy.call_sync(
                 'org.freedesktop.DBus.Properties.Get',
                 GLib.Variant('(ss)', (self._interface_name, name)),
                 Gio.DBusCallFlags.NONE,
                 DBUS_TIMEOUT,
                 None)
+            self.__stale.discard(name)
             return prop.unpack()[0]
         except GLib.Error as e:
-            property = self.__proxy.get_cached_property(name)
-            if property is not None:
-                return property.unpack()
-            elif name in self.__fallback:
+            # The refresh failed: fall back to the last cached value if we have
+            # one, but record that it is stale so callers can tell.
+            cached = self.__proxy.get_cached_property(name)
+            if cached is not None:
+                logging.debug(f"{self._interface_name}.{name}: serving cached value after "
+                              f"refresh error: {e.message}")
+                self.__stale.add(name)
+                return cached.unpack()
+            if name in self.__fallback:
                 return self.__fallback[name]
-            else:
-                raise parse_dbus_error(e)
+            raise parse_dbus_error(e)
 
     def set(self, name: str, value: str | int | bool) -> None:
         v = GLib.Variant(self.__variant_map[type(value)], value)
@@ -145,6 +196,9 @@ def get_properties(self) -> dict[str, Any]:
         return props
 
     def destroy(self) -> None:
+        registry = type(self).__dict__.get("_registry")
+        if registry is not None:
+            registry.remove(self.get_object_path())
         if self.__proxy:
             del self.__proxy
 

test/benchmarks/bench_bluez_base_get.py

@@ -0,0 +1,113 @@
+"""Benchmark for the bluez Base property-read path (perf-4).
+
+Before perf-4, ``Base.get`` issued one synchronous ``Properties.Get`` D-Bus
+round-trip on every read. After perf-4 it serves from the proxy's local cache
+(kept current by PropertiesChanged), so repeated reads of the same property
+cost zero round-trips.
+
+Each ``call_sync`` models one synchronous D-Bus round-trip (the dominant cost
+on this blocking path). Output is one JSON line comparing modeled round-trips
+for the old "always sync" behaviour against the current cached-first ``get``.
+"""
+from __future__ import annotations
+
+import json
+import sys
+import types
+from pathlib import Path
+from typing import Any
+from unittest.mock import patch
+
+import gi
+
+gi.require_version("Gtk", "3.0")
+from gi.repository import GLib  # noqa: E402
+
+_constants = types.ModuleType("blueman.Constants")
+for _name in ("BIN_DIR", "BLUETOOTHD_PATH", "ICON_PATH", "PIXMAP_PATH", "UI_PATH"):
+    setattr(_constants, _name, Path("/tmp"))
+sys.modules.setdefault("blueman.Constants", _constants)
+
+from blueman.bluemantyping import ObjectPath  # noqa: E402
+from blueman.bluez.Base import Base  # noqa: E402
+
+# Cost of one synchronous D-Bus round-trip on the system bus; conservative.
+DBUS_RTT = 0.0002  # 200 microseconds
+
+# A device row reads this many properties repeatedly while the manager is open.
+PROPS = ["Connected", "Paired", "Trusted", "Blocked", "Icon", "Class", "Address"]
+
+
+class CountingVariant:
+    def __init__(self, value: Any) -> None:
+        self._value = value
+
+    def unpack(self) -> Any:
+        return self._value
+
+
+class CountingProxy:
+    """Fake proxy counting synchronous round-trips, with a warm cache."""
+
+    def __init__(self, props: dict[str, Any]) -> None:
+        self._props = props
+        self._cached = dict(props)  # warm, as Gio loads + signal-updates it
+        self.sync_calls = 0
+
+    def connect(self, *_args: Any) -> int:
+        return 1
+
+    def get_object_path(self) -> str:
+        return "/org/bluez/hci0/dev_AA"
+
+    def get_interface_name(self) -> str:
+        return "org.bluez.Device1"
+
+    def get_cached_property(self, name: str) -> CountingVariant | None:
+        if name in self._cached:
+            return CountingVariant(self._cached[name])
+        return None
+
+    def call_sync(self, _method: str, params: Any, *_a: Any) -> CountingVariant:
+        self.sync_calls += 1
+        name = params.unpack()[1]
+        return CountingVariant((self._props[name],))
+
+
+class FakeBase(Base):
+    _interface_name = "org.bluez.Device1"
+
+
+def run(reads_per_prop: int) -> dict[str, Any]:
+    props = {p: (True if p in ("Connected", "Paired", "Trusted", "Blocked") else "x") for p in PROPS}
+    proxy = CountingProxy(props)
+    with patch("blueman.bluez.Base.Gio.DBusProxy.new_for_bus_sync", return_value=proxy):
+        obj = FakeBase(obj_path=ObjectPath("/org/bluez/hci0/dev_AA"))
+
+    total_reads = reads_per_prop * len(PROPS)
+    for _ in range(reads_per_prop):
+        for p in PROPS:
+            obj.get(p)
+
+    new_roundtrips = proxy.sync_calls          # cached-first: expected 0
+    old_roundtrips = total_reads               # always-sync: one per read
+    return {
+        "reads": total_reads,
+        "old_roundtrips": old_roundtrips,
+        "new_roundtrips": new_roundtrips,
+        "dbus_rtt": DBUS_RTT,
+        "old_modeled_seconds": old_roundtrips * DBUS_RTT,
+        "new_modeled_seconds": new_roundtrips * DBUS_RTT,
+        "roundtrips_saved_pct": (old_roundtrips - new_roundtrips) / old_roundtrips * 100,
+    }
+
+
+def _silence_glib_unused() -> None:
+    # GLib imported for parity with the module under test / future use.
+    assert GLib is not None
+
+
+if __name__ == "__main__":
+    _silence_glib_unused()
+    reads = int(sys.argv[1]) if len(sys.argv) > 1 else 5000
+    print(json.dumps(run(reads)))

test/bluez/Makefile.am

@@ -3,5 +3,6 @@ SUBDIRS =   \
 
 EXTRA_DIST =    \
     __init__.py \
+    test_base.py \
     test_imports.py \
     test_manager.py