From 6bf7c3fa5179d29ef2a9aad44e70b0e36fc4f49e Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Sat, 3 Oct 2026 12:11:27 -0400 Subject: [PATCH] perf(layout): id-keyed fit_image cache entries no longer pin the source (#732) LayoutContext.fit_image keyed images without a cache_key by id() and held a strong reference to the source so the id could not be recycled. A plugin following the documented one-liner -- draw_image(Image.open(path), box) each frame -- never hit that cache and kept the last 64 sources alive: ~64MB for 500x500 RGBA team logos (median size under assets/sports), up to ~600MB for the largest. The entry now holds a weak reference whose callback drops it when the source is freed, and a hit re-checks that the referent is the same image. Sources that cannot be weak-referenced are still pinned. Keyed entries (the only kind any plugin on ledmatrix-plugins main uses today: football-scoreboard's logo fit) are unchanged. Co-authored-by: Claude Opus 5.5 --- src/adaptive_layout.py | 46 ++++++++++++++++++++++++++---------- test/test_adaptive_images.py | 46 ++++++++++++++++++++++++++++++++---- 2 files changed, 75 insertions(+), 17 deletions(-) diff --git a/src/adaptive_layout.py b/src/adaptive_layout.py index df870052..334f7e3c 100644 --- a/src/adaptive_layout.py +++ b/src/adaptive_layout.py @@ -28,6 +28,7 @@ freetype.Face, so it drops straight into DisplayManager.draw_text(). """ import logging +import weakref from collections import OrderedDict from dataclasses import dataclass from typing import Any, Dict, List, Optional, Sequence, Tuple, Union @@ -332,9 +333,10 @@ class LayoutContext: # a plugin fitting changing text (a live game clock, a ticker) on a # 24/7 service would otherwise grow this without bound. self._fit_cache: "OrderedDict[Any, FitResult]" = OrderedDict() - # LRU-bounded (images are big). Entries hold a strong reference to - # the source image when keyed by id() so the id can't be recycled - # out from under the cache. + # LRU-bounded (images are big). An id()-keyed entry watches its + # source image through a weak reference and is dropped when the + # source is freed (see fit_image), so the id can't be recycled out + # from under the cache and the cache never keeps the source alive. self._image_cache: "OrderedDict[Any, Tuple[Any, Any]]" = OrderedDict() _IMAGE_CACHE_MAX = 64 @@ -536,8 +538,15 @@ class LayoutContext: cached per (image, box size, options) for this panel size. Prefer a stable ``cache_key`` (e.g. "logo:KC") for images that get - reloaded — the default id()-based key is safe (the entry pins the - source image) but misses across reloads of the same content. + reloaded — the default id()-based key misses across reloads of the + same content. + + An id()-keyed entry lives only as long as its source image: it holds + a weak reference and is dropped when the source is freed. It used to + pin the source instead, so a plugin passing a freshly loaded image + each frame (``draw_image(Image.open(path), box)``, the documented + one-liner) never hit and kept the last 64 sources alive — ~64MB for + 500x500 RGBA team logos, the median size under assets/sports. """ from src.adaptive_images import fit_image as _fit_image @@ -547,18 +556,31 @@ class LayoutContext: key = ("image", identity, img.size, box_w, box_h, mode, crop_to_ink, anchor, resample_name, upscale) - cached = self._image_cache.get(key) - if cached is not None: - self._image_cache.move_to_end(key) + cache = self._image_cache + cached = cache.get(key) + # An id()-keyed hit must still be this very image; the callback below + # normally removes a dead source's entry before its id can recur. + if cached is not None and (cache_key is not None or cached[1]() is img): + cache.move_to_end(key) return cached[0] result = _fit_image(img, (box_w, box_h), mode=mode, crop_to_ink=crop_to_ink, anchor=anchor, resample=resample, upscale=upscale) - # Pin the source only for id()-keyed entries (see docstring). - self._image_cache[key] = (result, img if cache_key is None else None) - while len(self._image_cache) > self._IMAGE_CACHE_MAX: - self._image_cache.popitem(last=False) + source = None + if cache_key is None: + def _forget(ref: Any, key: Any = key) -> None: + entry = cache.get(key) + if entry is not None and entry[1] is ref: + cache.pop(key, None) + try: + source = weakref.ref(img, _forget) + except TypeError: + # Not weak-referenceable: pin it, as before. + source = lambda img=img: img # noqa: E731 + cache[key] = (result, source) + while len(cache) > self._IMAGE_CACHE_MAX: + cache.popitem(last=False) return result # ---- text utilities ------------------------------------------------ diff --git a/test/test_adaptive_images.py b/test/test_adaptive_images.py index 5bf71c44..d60f675c 100644 --- a/test/test_adaptive_images.py +++ b/test/test_adaptive_images.py @@ -145,13 +145,49 @@ class TestContextImageCache: a = ctx.fit_image(img, (20, 20)) assert ctx.fit_image(img, (20, 20)) is a - def test_id_safety_pins_source(self, ctx): - # id()-keyed entries must pin the source image so a recycled id - # can't alias a dead image's cache entry. + def test_id_keyed_entry_does_not_pin_source(self, ctx): + import gc + import weakref + img = _solid(10, 10) ctx.fit_image(img, (20, 20)) - pinned = [entry[1] for entry in ctx._image_cache.values()] - assert img in pinned + assert len(ctx._image_cache) == 1 + watch = weakref.ref(img) + del img + gc.collect() + assert watch() is None # the cache did not keep it alive + assert len(ctx._image_cache) == 0 # and its entry went with it + + def test_fresh_image_each_frame_holds_nothing(self, ctx): + # draw_image(Image.open(path), box) every frame: the old pinning + # filled all 64 slots with dead-weight sources. + for _ in range(ctx._IMAGE_CACHE_MAX * 2): + ctx.fit_image(_solid(50, 50), (20, 20)) + assert len(ctx._image_cache) == 0 + + def test_recycled_id_does_not_alias(self, ctx): + # A same-size image at a recycled address must not get the dead + # image's fit, even if the entry somehow outlived its source. + red = _solid(10, 10, (255, 0, 0, 255)) + first = ctx.fit_image(red, (20, 20)) + key = next(iter(ctx._image_cache)) + entry = ctx._image_cache[key] + ctx._image_cache[key] = (entry[0], lambda: None) # source "gone" + again = ctx.fit_image(red, (20, 20)) + assert again is not first # refit, not a stale hit + assert ctx.fit_image(red, (20, 20)) is again + + def test_unweakrefable_source_is_pinned(self, ctx, monkeypatch): + import src.adaptive_layout as layout_mod + + def no_weakref(*_args, **_kwargs): + raise TypeError("cannot create weak reference") + + monkeypatch.setattr(layout_mod.weakref, "ref", no_weakref) + img = _solid(10, 10) + a = ctx.fit_image(img, (20, 20)) + assert ctx.fit_image(img, (20, 20)) is a + assert next(iter(ctx._image_cache.values()))[1]() is img def test_cache_key_entries_do_not_pin(self, ctx): img = _solid(10, 10)