diff --git a/src/korvid/ui/widgets/resource_table.py b/src/korvid/ui/widgets/resource_table.py index 8e124ebc..1bd5d5aa 100644 --- a/src/korvid/ui/widgets/resource_table.py +++ b/src/korvid/ui/widgets/resource_table.py @@ -3,7 +3,8 @@ from __future__ import annotations from collections.abc import Callable -from typing import cast +from datetime import UTC, datetime +from typing import Final, cast from rich.cells import cell_len from rich.text import Text @@ -34,6 +35,10 @@ #: Looks up live metrics for (namespace, name); None disables the join. MetricsLookup = Callable[[str, str], PodMetrics | None] +#: `_emit_row(stamp=...)` sentinel: this row opts out of the memo. Distinct +#: from `None`, which is a legitimate stamp (a row with no volatile inputs). +_NO_STAMP: Final = object() + _POD_COLS = ( "NAME", "READY", @@ -350,6 +355,16 @@ def on_mount(self) -> None: self._last_sort = None self._active_view = None self._pending_rows: list[tuple[str, list[str | Text]]] = [] + #: Per-row cell memo (issue #208): row key -> (summary, stamp, row). + #: Keeps a repaint proportional to the rows that actually changed. + self._row_memo: dict[str, tuple[Summary, object, tuple[str, list[str | Text]]]] = {} + #: Cell-set shape the memo was built for; a change invalidates it. + self._memo_signature: tuple[str, bool, ViewConfig | None] | None = None + #: Cells currently in the table, so the diff never has to read them + #: back out of the DataTable one `get_row` at a time. + self._emitted: dict[str, list[str | Text]] = {} + #: Single clock reading per repaint; see `show()`. + self._render_now: datetime = datetime.now(UTC) def show( self, @@ -380,6 +395,17 @@ def show( self._active_view, ) restore = self._cursor_snapshot() if same_view else None + # One clock reading per repaint: every AGE cell is derived from the + # same instant, and the memo below can compare age strings without + # each row racing its own `datetime.now()`. + self._render_now = datetime.now(UTC) + # The memo holds finished cell lists, so it is only valid while the + # cell set has the same shape. Sort is deliberately absent: it + # reorders rows and decorates headers, it does not change any cell. + signature = (kind, all_namespaces, view) + if signature != self._memo_signature: + self._row_memo.clear() + self._memo_signature = signature # Build the desired rows first (the builders only fill # `_pending_rows`), so a pure data refresh can be diffed against the # current table instead of clearing and rebuilding it. @@ -389,6 +415,7 @@ def show( kind, rows, all_namespaces=all_namespaces, pattern=pattern, metrics=metrics, sort=sort ) pending, self._pending_rows = self._pending_rows, [] + self._prune_memo(pending) if same_view and sort == self._last_sort and self._apply_in_place(pending): # Nothing was cleared, so the scroll offset never moved; only # the cursor may have slid when rows above it were removed — @@ -431,6 +458,7 @@ def show( self.clear() for key, cells in pending: self.add_row(*cells, key=key) + self._emitted = dict(pending) if restore is not None: # A background refresh must not scroll the cursor back into # view — the viewport restore below keeps the user's position. @@ -488,6 +516,14 @@ def _apply_in_place(self, pending: list[tuple[str, list[str | Text]]]) -> bool: place; returns False when the update needs the rebuild path (see `_in_place_plan` for the eligibility rules). + The comparison runs against `_emitted` — the cells this widget last + put into the table — rather than `DataTable.get_row()`, which re-derives + `ordered_columns` on every call and would make an untouched repaint + cost one lookup plus a full cell comparison per row (issue #208). + A memo hit re-emits the *same* cell list, so an unchanged row is + settled by one identity check. `get_row` remains the fallback whenever + `_emitted` has no record of a row. + Width updates are requested only for cells wider than their column: `update_width=True` is not grow-only — a narrower replacement rescans and *shrinks* the column, shifting the layout this path must keep @@ -502,16 +538,33 @@ def _apply_in_place(self, pending: list[tuple[str, list[str | Text]]]) -> bool: self.remove_row(key) columns = self.ordered_columns for key, cells in pending: - if key in current_set: - old_cells = self.get_row(key) - for column, old_cell, new_cell in zip(columns, old_cells, cells, strict=True): - if not _cells_equal(old_cell, new_cell): - grew = _cell_width(new_cell) > column.content_width - self.update_cell(key, column.key, new_cell, update_width=grew) - else: + if key not in current_set: self.add_row(*cells, key=key) + continue + old_cells = self._emitted.get(key) + if old_cells is cells: + continue # memo hit: same list object, nothing can have changed + if old_cells is None: + old_cells = self.get_row(key) + for column, old_cell, new_cell in zip(columns, old_cells, cells, strict=True): + if not _cells_equal(old_cell, new_cell): + grew = _cell_width(new_cell) > column.content_width + self.update_cell(key, column.key, new_cell, update_width=grew) + self._emitted = dict(pending) return True + def _prune_memo(self, pending: list[tuple[str, list[str | Text]]]) -> None: + """Drop memo entries for rows no longer rendered. + + Left alone the memo would grow for the lifetime of the session as pod + names churn. Pruning is amortised: it only runs once the memo has + drifted well past the rendered set, so the common repaint stays free. + """ + if len(self._row_memo) <= 4 * len(pending) + 1024: + return + live = {key for key, _ in pending} + self._row_memo = {key: entry for key, entry in self._row_memo.items() if key in live} + def _cursor_snapshot(self) -> tuple[str, int] | None: """(row key, row index) under the cursor, or None on an empty table.""" if self.row_count == 0 or self.cursor_row < 0: @@ -531,7 +584,14 @@ def _restore_cursor(self, key: str, index: int, *, scroll: bool = True) -> None: row = min(index, self.row_count - 1) self.move_cursor(row=row, animate=False, scroll=scroll) - def _emit_row(self, obj: Summary, cells: list[str | Text], *, all_namespaces: bool) -> None: + def _emit_row( + self, + obj: Summary, + cells: list[str | Text], + *, + all_namespaces: bool, + stamp: object = _NO_STAMP, + ) -> None: """Finish one row: apply the custom view (issue #45), prepend the namespace in all-namespaces mode, and buffer it keyed by ns/name for `show()` to diff or add. @@ -539,6 +599,12 @@ def _emit_row(self, obj: Summary, cells: list[str | Text], *, all_namespaces: bo *cells* is the kind's default cell list without the namespace. Rows whose summaries carry fewer custom values than configured (e.g. seeded before the config existed) pad with ``. + + *stamp* is everything the cells depend on that is *not* carried by the + frozen summary itself (the age string, plus the metrics sample on the + pods view). It is memoised with the row so `_reuse_row` can prove the + cells would come out identical; `_NO_STAMP` opts a caller out of the + memo entirely. """ view = self._active_view if view is not None: @@ -549,7 +615,37 @@ def _emit_row(self, obj: Summary, cells: list[str | Text], *, all_namespaces: bo cells = [cells[0], *extras] if view.replace else [*cells, *extras] if all_namespaces: cells.insert(0, obj.namespace) - self._pending_rows.append((f"{obj.namespace}/{obj.name}", cells)) + row = (f"{obj.namespace}/{obj.name}", cells) + self._pending_rows.append(row) + if stamp is not _NO_STAMP: + self._row_memo[row[0]] = (obj, stamp, row) + + def _reuse_row(self, obj: Summary, stamp: object) -> bool: + """Re-emit *obj*'s memoised row when nothing feeding its cells changed. + + Watch events replace the whole frozen summary, so an identity hit + proves every summary-derived cell is unchanged; *stamp* covers the + rest. Returns False when the row must be rebuilt. + """ + entry = self._row_memo.get(f"{obj.namespace}/{obj.name}") + if entry is None or entry[0] is not obj or entry[1] != stamp: + return False + self._pending_rows.append(entry[2]) + return True + + def _stamp(self, volatile: object) -> object: + """The volatile inputs that actually reach the emitted row. + + A `replace: true` custom view (issue #45) keeps only NAME plus the + configured custom values, all of which come from the frozen summary — + so the AGE and metrics cells it discards must not be allowed to + invalidate the memo, or a minute rollover or metrics poll would rebuild + cells nobody can see. + """ + view = self._active_view + if view is not None and view.replace: + return None + return volatile def _render_rows( self, @@ -621,10 +717,17 @@ def _add_pod_rows( pods = cast(list[PodSummary], rows) if not presorted: pods = sorted(pods, key=_pod_sort_key) + now = self._render_now for pod in pods: if pattern and pattern.lower() not in pod.name.lower(): continue usage = metrics(pod.namespace, pod.name) if metrics is not None else None + age = pod.age(now) + # Everything else on this row is derived from the frozen summary, + # so the live metrics sample and the age are the whole stamp. + stamp = self._stamp((age, usage)) + if self._reuse_row(pod, stamp): + continue cells: list[str | Text] = [ pod.name, _ready_cell(pod.ready), @@ -634,10 +737,10 @@ def _add_pod_rows( f"{pod.cpu_request}/{pod.cpu_limit}", f"{pod.mem_request}/{pod.mem_limit}", Text(pod.qos, style=_QOS_STYLE.get(pod.qos, "dim")), - pod.age(), + age, pod.node or "-", ] - self._emit_row(pod, cells, all_namespaces=all_namespaces) + self._emit_row(pod, cells, all_namespaces=all_namespaces, stamp=stamp) def _add_replicaset_rows( self, rows: list[Summary], *, all_namespaces: bool, pattern: str, presorted: bool = False @@ -664,18 +767,25 @@ def _add_replicaset_rows( if pattern and pattern.lower() not in obj.name.lower(): continue if isinstance(obj, ReplicaSetSummary): + age = obj.age(self._render_now) + stamp = self._stamp(age) + if self._reuse_row(obj, stamp): + continue cells: list[str | Text] = [ obj.name, obj.revision, str(obj.desired), str(obj.current), _ready_cell(obj.ready), - obj.age(), + age, ] else: - age = obj.age() if isinstance(obj, GenericSummary) else "" + age = obj.age(self._render_now) if isinstance(obj, GenericSummary) else "" + stamp = self._stamp(age) + if self._reuse_row(obj, stamp): + continue cells = [obj.name, "", "", "", "", age] - self._emit_row(obj, cells, all_namespaces=all_namespaces) + self._emit_row(obj, cells, all_namespaces=all_namespaces, stamp=stamp) def _add_helm_release_rows( self, rows: list[Summary], *, all_namespaces: bool, pattern: str, presorted: bool = False @@ -686,15 +796,19 @@ def _add_helm_release_rows( for rel in releases: if pattern and pattern.lower() not in rel.name.lower(): continue + age = rel.age(self._render_now) + stamp = self._stamp(age) + if self._reuse_row(rel, stamp): + continue cells: list[str | Text] = [ rel.name, str(rel.revision), _helm_status_cell(rel.status), rel.chart, rel.app_version, - rel.age(), + age, ] - self._emit_row(rel, cells, all_namespaces=all_namespaces) + self._emit_row(rel, cells, all_namespaces=all_namespaces, stamp=stamp) def _add_helm_revision_rows( self, rows: list[Summary], *, all_namespaces: bool, pattern: str, presorted: bool = False @@ -706,6 +820,10 @@ def _add_helm_revision_rows( for rev in revisions: if pattern and pattern.lower() not in rev.name.lower(): continue + age = rev.age(self._render_now) + stamp = self._stamp(age) + if self._reuse_row(rev, stamp): + continue cells: list[str | Text] = [ rev.name, str(rev.revision), @@ -713,9 +831,9 @@ def _add_helm_revision_rows( rev.chart, rev.app_version, rev.description, - rev.age(), + age, ] - self._emit_row(rev, cells, all_namespaces=all_namespaces) + self._emit_row(rev, cells, all_namespaces=all_namespaces, stamp=stamp) def _add_fallback_rows( self, @@ -734,9 +852,12 @@ def _add_fallback_rows( for obj in rows: if pattern and pattern.lower() not in obj.name.lower(): continue - age = obj.age() if isinstance(obj, GenericSummary) else "" + age = obj.age(self._render_now) if isinstance(obj, GenericSummary) else "" + stamp = self._stamp(age) + if self._reuse_row(obj, stamp): + continue cells: list[str | Text] = [obj.name, *[""] * (width - 2), age] - self._emit_row(obj, cells, all_namespaces=all_namespaces) + self._emit_row(obj, cells, all_namespaces=all_namespaces, stamp=stamp) def _add_package_rows( self, rows: list[Summary], *, all_namespaces: bool, pattern: str, presorted: bool = False @@ -747,15 +868,19 @@ def _add_package_rows( for pkg in packages: if pattern and pattern.lower() not in pkg.name.lower(): continue + age = pkg.age(self._render_now) + stamp = self._stamp(age) + if self._reuse_row(pkg, stamp): + continue cells: list[str | Text] = [ pkg.name, pkg.catalog or "-", pkg.default_channel or "-", ",".join(pkg.channels) or "-", pkg.description or "-", - pkg.age(), + age, ] - self._emit_row(pkg, cells, all_namespaces=all_namespaces) + self._emit_row(pkg, cells, all_namespaces=all_namespaces, stamp=stamp) fallbacks = [r for r in rows if not isinstance(r, PackageManifestSummary)] self._add_fallback_rows( fallbacks, @@ -774,15 +899,19 @@ def _add_subscription_rows( for sub in subs: if pattern and pattern.lower() not in sub.name.lower(): continue + age = sub.age(self._render_now) + stamp = self._stamp(age) + if self._reuse_row(sub, stamp): + continue cells: list[str | Text] = [ sub.name, sub.channel or "-", sub.source or "-", sub.installed_csv or "-", sub.state or "-", - sub.age(), + age, ] - self._emit_row(sub, cells, all_namespaces=all_namespaces) + self._emit_row(sub, cells, all_namespaces=all_namespaces, stamp=stamp) fallbacks = [r for r in rows if not isinstance(r, OLMSubscriptionSummary)] self._add_fallback_rows( fallbacks, @@ -801,14 +930,18 @@ def _add_csv_rows( for csv in csvs: if pattern and pattern.lower() not in csv.name.lower(): continue + age = csv.age(self._render_now) + stamp = self._stamp(age) + if self._reuse_row(csv, stamp): + continue cells: list[str | Text] = [ csv.name, csv.display_name or "-", csv.version or "-", _csv_phase_cell(csv.phase), - csv.age(), + age, ] - self._emit_row(csv, cells, all_namespaces=all_namespaces) + self._emit_row(csv, cells, all_namespaces=all_namespaces, stamp=stamp) fallbacks = [r for r in rows if not isinstance(r, CSVSummary)] self._add_fallback_rows( fallbacks, @@ -827,5 +960,9 @@ def _add_generic_rows( for obj in generics: if pattern and pattern.lower() not in obj.name.lower(): continue - cells: list[str | Text] = [obj.name, obj.age()] - self._emit_row(obj, cells, all_namespaces=all_namespaces) + age = obj.age(self._render_now) + stamp = self._stamp(age) + if self._reuse_row(obj, stamp): + continue + cells: list[str | Text] = [obj.name, age] + self._emit_row(obj, cells, all_namespaces=all_namespaces, stamp=stamp) diff --git a/tests/ui/test_table_row_memo.py b/tests/ui/test_table_row_memo.py new file mode 100644 index 00000000..3a723bbc --- /dev/null +++ b/tests/ui/test_table_row_memo.py @@ -0,0 +1,233 @@ +"""Row-cell memo (issue #208): a repaint must cost work proportional to the +rows that actually changed, not to the total row count. + +Watch summaries are frozen dataclasses replaced wholesale, so a row whose +summary object is unchanged provably renders identical cells — its cells are +reused instead of rebuilt, and the in-place diff skips it without asking the +DataTable what it already holds. +""" + +from __future__ import annotations + +from dataclasses import replace +from datetime import UTC, datetime, timedelta +from typing import Any +from unittest import mock + +from korvid.core.config import KorvidConfig, ViewConfig +from korvid.core.store import Summary +from korvid.k8s.columns import CustomColumn +from korvid.k8s.metrics import PodMetrics +from korvid.k8s.models import PodSummary +from korvid.ui.widgets import resource_table +from korvid.ui.widgets.resource_table import ResourceTable + +from .test_app import _pod, make_app +from .waits import until + + +class _FrozenClock: + """Stand-in for `datetime` so a repaint can be aged at a chosen instant.""" + + def __init__(self, instant: datetime) -> None: + self._instant = instant + + def now(self, tz: Any = None) -> datetime: + return self._instant + + +def _spy_emit(table: ResourceTable) -> list[str]: + """Record the row key of every row whose cells are actually rebuilt.""" + built: list[str] = [] + original = table._emit_row + + def spy(obj: Any, cells: Any, **kwargs: Any) -> Any: + built.append(f"{obj.namespace}/{obj.name}") + return original(obj, cells, **kwargs) + + table._emit_row = spy # type: ignore[method-assign] # test spy + return built + + +def _spy_get_row(table: ResourceTable) -> list[str]: + """Record the diff's own `get_row` lookups. + + Textual calls `get_row` with a `RowKey` while painting; only the diff + looks rows up by their string key, so filtering on `str` isolates it. + """ + looked_up: list[str] = [] + original = table.get_row + + def spy(row_key: Any) -> Any: + if isinstance(row_key, str): + looked_up.append(row_key) + return original(row_key) + + table.get_row = spy # type: ignore[method-assign] # test spy + return looked_up + + +async def test_repaint_rebuilds_only_the_changed_row() -> None: + """One MODIFIED pod must rebuild one row's cells, not every row's.""" + app = make_app([_pod("alpha"), _pod("beta"), _pod("gamma")]) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 3, label="pods loaded") + built = _spy_emit(table) + app.store.apply_event("pods", "default", "MODIFIED", _pod("beta", phase="Failed")) + await until( + pilot, + lambda: str(table.get_row("default/beta")[2]) == "Failed", + label="phase cell updated", + ) + assert built == ["default/beta"] + + +async def test_unchanged_repaint_rebuilds_nothing() -> None: + """A repaint with the same summary objects must not rebuild any cells.""" + app = make_app([_pod("alpha"), _pod("beta")]) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 2, label="pods loaded") + built = _spy_emit(table) + table.show("pods", app.store.get("pods", "default"), all_namespaces=False, pattern="") + assert built == [] + + +async def test_unchanged_rows_are_not_read_back_from_the_datatable() -> None: + """The diff must settle rows from what this widget last emitted, never by + reading them back out of the DataTable one `get_row` at a time.""" + app = make_app([_pod("alpha"), _pod("beta"), _pod("gamma")]) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 3, label="pods loaded") + read_row = table.get_row + looked_up = _spy_get_row(table) + app.store.apply_event("pods", "default", "MODIFIED", _pod("beta", phase="Failed")) + await until( + pilot, + lambda: str(read_row("default/beta")[2]) == "Failed", + label="phase cell updated", + ) + assert looked_up == [] + + +async def test_changed_metrics_rebuild_the_row_without_a_new_summary() -> None: + """Usage cells come from the metrics sample, not the summary: a new sample + for the same pod object must still re-render that row.""" + samples: dict[tuple[str, str], PodMetrics] = {} + + def lookup(namespace: str, name: str) -> PodMetrics | None: + return samples.get((namespace, name)) + + app = make_app([_pod("alpha")]) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 1, label="pods loaded") + rows = app.store.get("pods", "default") + table.show("pods", rows, all_namespaces=False, pattern="", metrics=lookup) + built = _spy_emit(table) + samples[("default", "alpha")] = PodMetrics( + name="alpha", namespace="default", cpu_cores=0.5, memory_bytes=1024 * 1024 + ) + table.show("pods", rows, all_namespaces=False, pattern="", metrics=lookup) + assert built == ["default/alpha"] + assert str(table.get_row("default/alpha")[4]) != "-" + + +async def test_view_signature_change_discards_the_memo() -> None: + """Switching to the all-namespaces column set must rebuild every row: the + memoised cells were built without the NAMESPACE column.""" + app = make_app([_pod("alpha"), _pod("beta")]) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 2, label="pods loaded") + built = _spy_emit(table) + table.show("pods", app.store.get("pods", "default"), all_namespaces=True, pattern="") + assert built == ["default/alpha", "default/beta"] + assert str(table.get_row("default/alpha")[0]) == "default" + + +async def test_age_uses_one_clock_reading_per_repaint() -> None: + """Every row in a repaint is aged against the same instant, so a repaint + that straddles a second boundary cannot report inconsistent ages.""" + import korvid.k8s.models as models + + app = make_app([_pod("alpha"), _pod("beta"), _pod("gamma")]) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 3, label="pods loaded") + original = models.format_age + seen: list[Any] = [] + + def counting_format_age(value: str, now: Any = None) -> str: + seen.append(now) + return original(value, now=now) + + models.format_age = counting_format_age # type: ignore[assignment] # test spy + try: + # Fresh summary objects: identity differs, so every row rebuilds. + fresh: list[Summary] = [_pod("alpha"), _pod("beta"), _pod("gamma")] + table.show("pods", fresh, all_namespaces=False, pattern="") + finally: + models.format_age = original + assert len(seen) >= 3, seen + assert all(now is not None for now in seen), seen + assert len(set(seen)) == 1, seen + + +async def test_age_refreshes_for_an_unchanged_summary_as_time_passes() -> None: + """The memo must not freeze AGE: a pod nobody touches still ages, so the + stamp has to carry the age string and re-render the row when it rolls over.""" + created = (datetime.now(UTC) - timedelta(minutes=5)).isoformat().replace("+00:00", "Z") + pod = PodSummary( + name="alpha", + namespace="default", + phase="Running", + ready="1/1", + restarts=0, + node=None, + created=created, + ) + app = make_app([pod]) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 1, label="pod loaded") + rows: list[Summary] = [pod] + table.show("pods", rows, all_namespaces=False, pattern="") + assert str(table.get_row("default/alpha")[-2]) == "5m" + # Same summary object, clock advanced past the next minute boundary. + with mock.patch.object( + resource_table, "datetime", _FrozenClock(datetime.now(UTC) + timedelta(minutes=1)) + ): + table.show("pods", rows, all_namespaces=False, pattern="") + assert str(table.get_row("default/alpha")[-2]) == "6m" + + +async def test_replace_view_rows_ignore_hidden_volatile_cells() -> None: + """A `replace: true` custom view keeps only NAME plus the configured + values — all carried by the frozen summary. Nothing volatile survives into + the row, so a metrics poll must not rebuild cells the view discards.""" + samples: dict[tuple[str, str], PodMetrics] = {} + + def lookup(namespace: str, name: str) -> PodMetrics | None: + return samples.get((namespace, name)) + + config = KorvidConfig( + namespace="default", + views={"pods": ViewConfig(columns=(CustomColumn("TEAM", "label", "team"),), replace=True)}, + ) + app = make_app([replace(_pod("alpha"), custom=("payments",))], config=config) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 1, label="pods loaded") + view = config.views["pods"] + rows = app.store.get("pods", "default") + table.show("pods", rows, all_namespaces=False, pattern="", metrics=lookup, view=view) + built = _spy_emit(table) + samples[("default", "alpha")] = PodMetrics( + name="alpha", namespace="default", cpu_cores=0.5, memory_bytes=1024 * 1024 + ) + table.show("pods", rows, all_namespaces=False, pattern="", metrics=lookup, view=view) + assert built == [] + assert [str(cell) for cell in table.get_row("default/alpha")] == ["alpha", "payments"]