perf(gui): clear DeviceList in one pass and fix discovery timer lifecycle

blueman/gui/DeviceList.py

@@ -68,6 +68,7 @@ def __init__(self, adapter_name: str | None = None, tabledata: list[ListDataDict
         self.__adapter_path: ObjectPath | None = None
         self.Adapter: Adapter | None = None
         self.discovering = False
+        self._discovery_timeout: int | None = None
 
         data = tabledata + [
             {"id": "device", "type": object},
@@ -206,6 +207,9 @@ def set_adapter(self, adapter: ObjectPath | str | None = None) -> None:
 
     def update_progress(self, time: float, totaltime: float) -> bool:
         if not self.discovering:
+            # The timer is ending itself; drop the id so stop_discovery does not
+            # try to remove an already-finished (and possibly reused) source.
+            self._discovery_timeout = None
             return False
 
         self.__discovery_time += time
@@ -214,6 +218,7 @@ def update_progress(self, time: float, totaltime: float) -> bool:
         if progress >= 1.0:
             progress = 1.0
         if self.__discovery_time >= totaltime:
+            self._discovery_timeout = None
             self.stop_discovery()
             return False
 
@@ -253,7 +258,7 @@ def discover_devices(self, time: float = 60.0,
                 self.Adapter.start_discovery(error_handler=error_handler)
                 self.discovering = True
                 t = 1.0 / 15 * 1000
-                GLib.timeout_add(int(t), self.update_progress, t / 1000, time)
+                self._discovery_timeout = GLib.timeout_add(int(t), self.update_progress, t / 1000, time)
 
     def is_valid_adapter(self) -> bool:
         if self.Adapter is None:
@@ -264,8 +269,14 @@ def is_valid_adapter(self) -> bool:
     def get_adapter_path(self) -> ObjectPath | None:
         return self.__adapter_path if self.is_valid_adapter() else None
 
+    def _remove_discovery_timeout(self) -> None:
+        if self._discovery_timeout is not None:
+            GLib.source_remove(self._discovery_timeout)
+            self._discovery_timeout = None
+
     def stop_discovery(self) -> None:
         self.discovering = False
+        self._remove_discovery_timeout()
         if self.Adapter is not None:
             self.Adapter.stop_discovery()
 
@@ -278,16 +289,15 @@ def get_selected_device(self) -> Device | None:
         return None
 
     def clear(self) -> None:
+        # Release the TreeRowReference cache *before* clearing the store. GTK
+        # keeps every live reference in sync as rows are removed, so clearing
+        # with the cache still populated makes liststore.clear() O(n^2); dropping
+        # the references first lets it run in O(n).
+        self.path_to_row = {}
         if len(self.liststore):
-            for i in self.liststore:
-                tree_iter = i.iter
-                dbus_path = self.get(tree_iter, "dbus_path")["dbus_path"]
-                self.device_remove_event(dbus_path)
             self.liststore.clear()
             self.emit("device-selected", None, None)
 
-        self.path_to_row = {}
-
     def find_device_by_path(self, object_path: ObjectPath) -> Gtk.TreeIter | None:
         row = self.path_to_row.get(object_path, None)
         if row is None:

test/benchmarks/bench_devicelist_clear.py

@@ -0,0 +1,90 @@
+"""Benchmark for DeviceList.clear() (perf-3 and vec-3).
+
+Three strategies on a real Gtk.ListStore plus a path_to_row cache of one live
+Gtk.TreeRowReference per row (as DeviceList keeps):
+
+- original : remove each row individually (each delete validates the iter and
+             updates every live reference) and then clear() — two O(n^2) passes
+             plus a mutation-while-iterating bug.
+- perf-3   : a single liststore.clear() while the references are still alive —
+             drops the redundant per-row loop, but clear() must still update
+             every live reference, so it is still ~O(n^2).
+- vec-3    : release the reference cache *before* clear() so GTK has nothing to
+             keep in sync — clear() becomes ~O(n).
+
+Output is one JSON blob with per-size timings and the 2x growth factor for each
+strategy (~4 means quadratic, ~2 means linear).
+"""
+from __future__ import annotations
+
+import json
+import sys
+import time
+
+import gi
+
+gi.require_version("Gtk", "3.0")
+from gi.repository import Gtk  # noqa: E402
+
+SIZES = [500, 1000, 2000, 4000]
+
+
+def _populate(n: int) -> tuple[Gtk.ListStore, dict[str, Gtk.TreeRowReference]]:
+    store = Gtk.ListStore(str)
+    refs: dict[str, Gtk.TreeRowReference] = {}
+    for i in range(n):
+        path = f"/org/bluez/hci0/dev_{i}"
+        tree_iter = store.append([path])
+        refs[path] = Gtk.TreeRowReference.new(store, store.get_path(tree_iter))
+    return store, refs
+
+
+def _original(store: Gtk.ListStore, refs: dict[str, Gtk.TreeRowReference]) -> None:
+    for path in list(refs):
+        ref = refs[path]
+        if ref.valid():
+            tree_path = ref.get_path()
+            assert tree_path is not None
+            store.remove(store.get_iter(tree_path))
+        del refs[path]
+    store.clear()
+
+
+def _perf3(store: Gtk.ListStore, refs: dict[str, Gtk.TreeRowReference]) -> None:
+    store.clear()
+    refs.clear()
+
+
+def _vec3(store: Gtk.ListStore, refs: dict[str, Gtk.TreeRowReference]) -> None:
+    refs.clear()
+    store.clear()
+
+
+STRATEGIES = {"original": _original, "perf3": _perf3, "vec3": _vec3}
+
+
+def run() -> dict[str, object]:
+    timings: dict[str, list[float]] = {name: [] for name in STRATEGIES}
+    for n in SIZES:
+        for name, fn in STRATEGIES.items():
+            store, refs = _populate(n)
+            start = time.perf_counter()
+            fn(store, refs)
+            timings[name].append(time.perf_counter() - start)
+
+    def growth(name: str) -> float:
+        t = timings[name]
+        return t[-1] / t[-2] if t[-2] else float("inf")
+
+    return {
+        "sizes": SIZES,
+        "seconds": {name: [round(t, 6) for t in ts] for name, ts in timings.items()},
+        "growth_2x": {name: round(growth(name), 2) for name in STRATEGIES},
+        "vec3_speedup_vs_original_at_max": round(
+            timings["original"][-1] / timings["vec3"][-1], 1),
+    }
+
+
+if __name__ == "__main__":
+    json.dump(run(), sys.stdout, indent=2)
+    sys.stdout.write("\n")

test/gui/Makefile.am

@@ -4,4 +4,5 @@ SUBDIRS =   \
 
 EXTRA_DIST =    \
     __init__.py \
+    test_devicelist.py \
     test_imports.py

test/gui/test_devicelist.py

@@ -0,0 +1,161 @@
+from pathlib import Path
+import sys
+import types
+from typing import Any
+from unittest import TestCase
+from unittest.mock import Mock, patch
+
+import gi
+
+gi.require_version("Gtk", "3.0")
+from gi.repository import Gtk  # noqa: E402
+
+constants: Any = types.ModuleType("blueman.Constants")
+constants.BIN_DIR = Path("/tmp")
+constants.BLUETOOTHD_PATH = Path("/tmp/bluetoothd")
+constants.ICON_PATH = Path("/tmp")
+constants.PIXMAP_PATH = Path("/tmp")
+constants.UI_PATH = Path("/tmp")
+sys.modules.setdefault("blueman.Constants", constants)
+
+from blueman.gui.DeviceList import DeviceList  # noqa: E402
+
+
+class FakeDeviceList:
+    """Bind the DeviceList methods under test onto a minimal fake self."""
+
+    discover_devices = DeviceList.discover_devices
+    stop_discovery = DeviceList.stop_discovery
+    update_progress = DeviceList.update_progress
+    _remove_discovery_timeout = DeviceList._remove_discovery_timeout
+    clear = DeviceList.clear
+
+    def __init__(self, liststore: Gtk.ListStore | None = None) -> None:
+        self.discovering = False
+        self._discovery_timeout: int | None = None
+        self.Adapter: Any = Mock()
+        self.liststore = liststore if liststore is not None else Gtk.ListStore(str)
+        self.path_to_row: dict[str, object] = {}
+        self.emitted: list[tuple[Any, ...]] = []
+
+    def emit(self, *args: Any) -> None:
+        self.emitted.append(args)
+
+
+class TestDiscoveryTimeout(TestCase):
+    def test_discover_stores_timeout_source(self) -> None:
+        fake = FakeDeviceList()
+        with patch("blueman.gui.DeviceList.GLib.timeout_add", return_value=77) as ta:
+            fake.discover_devices(60.0)
+        ta.assert_called_once()
+        self.assertEqual(fake._discovery_timeout, 77)
+        self.assertTrue(fake.discovering)
+
+    def test_discover_noop_when_already_discovering(self) -> None:
+        fake = FakeDeviceList()
+        fake.discovering = True
+        with patch("blueman.gui.DeviceList.GLib.timeout_add", return_value=77) as ta:
+            fake.discover_devices(60.0)
+        ta.assert_not_called()
+
+    def test_discover_noop_without_adapter(self) -> None:
+        fake = FakeDeviceList()
+        fake.Adapter = None
+        with patch("blueman.gui.DeviceList.GLib.timeout_add", return_value=77) as ta:
+            fake.discover_devices(60.0)
+        ta.assert_not_called()
+        self.assertIsNone(fake._discovery_timeout)
+
+    def test_stop_discovery_removes_source(self) -> None:
+        fake = FakeDeviceList()
+        fake.discovering = True
+        fake._discovery_timeout = 77
+        with patch("blueman.gui.DeviceList.GLib.source_remove") as sr:
+            fake.stop_discovery()
+        sr.assert_called_once_with(77)
+        self.assertIsNone(fake._discovery_timeout)
+        self.assertFalse(fake.discovering)
+        fake.Adapter.stop_discovery.assert_called_once_with()
+
+    def test_stop_discovery_without_source_is_noop(self) -> None:
+        fake = FakeDeviceList()
+        with patch("blueman.gui.DeviceList.GLib.source_remove") as sr:
+            fake.stop_discovery()
+        sr.assert_not_called()
+
+    def test_update_progress_not_discovering_drops_id(self) -> None:
+        fake = FakeDeviceList()
+        fake.discovering = False
+        fake._discovery_timeout = 77
+        self.assertFalse(fake.update_progress(0.1, 60.0))
+        self.assertIsNone(fake._discovery_timeout)
+
+    def test_update_progress_completion_stops_without_double_remove(self) -> None:
+        fake = FakeDeviceList()
+        fake.discovering = True
+        fake._discovery_timeout = 77
+        setattr(fake, "_DeviceList__discovery_time", 60.0)
+        with patch("blueman.gui.DeviceList.GLib.source_remove") as sr:
+            result = fake.update_progress(1.0, 60.0)
+        self.assertFalse(result)
+        self.assertIsNone(fake._discovery_timeout)
+        # id was nulled before stop_discovery, so no source_remove on the
+        # currently-running source.
+        sr.assert_not_called()
+        self.assertFalse(fake.discovering)
+
+    def test_update_progress_midway_keeps_running(self) -> None:
+        fake = FakeDeviceList()
+        fake.discovering = True
+        fake._discovery_timeout = 77
+        setattr(fake, "_DeviceList__discovery_time", 0.0)
+        self.assertTrue(fake.update_progress(1.0, 60.0))
+        self.assertEqual(fake._discovery_timeout, 77)
+        self.assertTrue(any(e[0] == "discovery-progress" for e in fake.emitted))
+
+
+def _fill(fake: FakeDeviceList, n: int) -> None:
+    for i in range(n):
+        path = f"/org/bluez/hci0/dev_{i}"
+        tree_iter = fake.liststore.append([path])
+        fake.path_to_row[path] = Gtk.TreeRowReference.new(
+            fake.liststore, fake.liststore.get_path(tree_iter))
+
+
+class TestClear(TestCase):
+    def test_clear_empties_store_and_cache(self) -> None:
+        fake = FakeDeviceList()
+        _fill(fake, 5)
+        fake.clear()
+        self.assertEqual(len(fake.liststore), 0)
+        self.assertEqual(fake.path_to_row, {})
+
+    def test_clear_emits_device_deselected_when_nonempty(self) -> None:
+        fake = FakeDeviceList()
+        _fill(fake, 3)
+        fake.clear()
+        self.assertIn(("device-selected", None, None), fake.emitted)
+
+    def test_clear_empty_store_no_emit(self) -> None:
+        fake = FakeDeviceList()
+        fake.path_to_row = {"stale": object()}  # type: ignore[dict-item]
+        fake.clear()
+        self.assertEqual(fake.path_to_row, {})
+        self.assertEqual(fake.emitted, [])
+
+    def test_clear_releases_all_references(self) -> None:
+        fake = FakeDeviceList()
+        _fill(fake, 10)
+        refs = list(fake.path_to_row.values())
+        fake.clear()
+        # After clear the references are dropped from the cache and invalid.
+        self.assertFalse(any(r.valid() for r in refs))  # type: ignore[attr-defined]
+
+    def test_clear_fuzz_sizes(self) -> None:
+        for n in (0, 1, 2, 17, 200, 1000):
+            with self.subTest(n=n):
+                fake = FakeDeviceList()
+                _fill(fake, n)
+                fake.clear()
+                self.assertEqual(len(fake.liststore), 0)
+                self.assertEqual(fake.path_to_row, {})