From 7a5cb1eff96a71c1fb32635379791065939fc912 Mon Sep 17 00:00:00 2001 From: Erik Welch Date: Mon, 6 Jul 2026 16:17:00 -0500 Subject: [PATCH] Fix gh-559 memory growth: build .ss lazily instead of storing it Every Matrix and Vector was born inside two reference cycles: the stored self.ss held _parent back to the object, and so did ss.config. Instances therefore died by the cyclic garbage collector rather than by refcount, and their C-side GrB buffers accumulated between gc sweeps. With gc disabled, a scaled version of the gh-559 batched-mxm loop grew without bound. The .ss namespace is now built per access (a property inside the existing class_property), nothing is stored on the parent, no cycle forms, and objects free as soon as their refcount drops. Two user-visible behavior changes: A.ss is A.ss is now False. It was True, because .ss was a stored attribute; each access now returns a fresh namespace object. Code that compares .ss by identity, or that caches attributes on it, will see the difference. Assigning A.ss now raises AttributeError. It previously succeeded and silently replaced the namespace, because "ss" was in __slots__. Class-level Matrix.ss and Vector.ss still resolve to the ss class, so the import_* classmethods are unchanged. Building the namespace per access costs roughly 215ns against roughly 78ns for the stored attribute (timeit), so about 1% of a single small mxm plus to_dense iteration, which runs about 15us. The .ss namespace is typically touched once per user operation. Six of the eight new tests fail without the fix. They assert on the reference graph rather than on process memory, which a functional suite cannot see and which an RSS delta would measure only statistically: a weakref must be dead the instant the last strong reference drops with the cyclic collector switched off, and a batched mxm loop must not raise the number of live Matrix objects reported by gc.get_objects(). The remaining two cover invariants the fix has to preserve, class-level access and views whose _parent is set, so they pass either way. --- graphblas/core/matrix.py | 20 ++-- graphblas/core/vector.py | 17 ++-- graphblas/tests/test_ss_refcount.py | 144 ++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+), 14 deletions(-) create mode 100644 graphblas/tests/test_ss_refcount.py diff --git a/graphblas/core/matrix.py b/graphblas/core/matrix.py index 698bf7b4f..ff04fe1e4 100644 --- a/graphblas/core/matrix.py +++ b/graphblas/core/matrix.py @@ -180,7 +180,7 @@ class Matrix(BaseType): """ - __slots__ = "_nrows", "_ncols", "_parent", "ss" + __slots__ = "_nrows", "_ncols", "_parent" ndim = 2 _is_transposed = False _name_counter = itertools.count() @@ -198,8 +198,6 @@ def __new__(cls, dtype=FP64, nrows=0, ncols=0, *, name=None): self._nrows = nrows.value self._ncols = ncols.value self._parent = None - if backend == "suitesparse": - self.ss = ss(self) return self @classmethod @@ -211,8 +209,6 @@ def _from_obj(cls, gb_obj, dtype, nrows, ncols, *, parent=None, name=None): self._nrows = nrows self._ncols = ncols self._parent = parent - if backend == "suitesparse": - self.ss = ss(self) return self def __del__(self): @@ -3532,10 +3528,20 @@ def _delete_element(self, resolved_indexes): if backend == "suitesparse": - Matrix.ss = class_property(Matrix.ss, ss) + # `.ss` is built lazily per access rather than stored on the instance: a + # stored `ss(self)` holds `_parent` back to this Matrix (and so does its + # config), forming a reference cycle that keeps the object (and its C-side + # GrB buffer) alive until the cyclic gc runs instead of dying by refcount. + # See gh-559. Class-level `Matrix.ss` still resolves to the `ss` class so + # the `import_*` classmethods keep working. + def _ss(self): + return ss(self) + + _ss.__name__ = _ss.__qualname__ = "ss" + Matrix.ss = class_property(property(_ss), ss) else: Matrix.ss = class_property( - Matrix.ss, 'ss attribute is only available with "suitesparse" backend', exceptional=True + property(), 'ss attribute is only available with "suitesparse" backend', exceptional=True ) diff --git a/graphblas/core/vector.py b/graphblas/core/vector.py index 8c73ecc48..b3301e915 100644 --- a/graphblas/core/vector.py +++ b/graphblas/core/vector.py @@ -152,7 +152,7 @@ class Vector(BaseType): """ - __slots__ = "_size", "_parent", "ss" + __slots__ = "_size", "_parent" ndim = 1 _name_counter = itertools.count() @@ -165,8 +165,6 @@ def __new__(cls, dtype=FP64, size=0, *, name=None): call("GrB_Vector_new", [_Pointer(self), self.dtype, size]) self._size = size.value self._parent = None - if backend == "suitesparse": - self.ss = ss(self) return self @classmethod @@ -177,8 +175,6 @@ def _from_obj(cls, gb_obj, dtype, size, *, parent=None, name=None): self.dtype = dtype self._size = size self._parent = parent - if backend == "suitesparse": - self.ss = ss(self) return self def __del__(self): @@ -2109,10 +2105,17 @@ def to_dict(self): if backend == "suitesparse": - Vector.ss = class_property(Vector.ss, ss) + # Built lazily per access, not stored, to avoid the ss/_parent reference + # cycle that would keep every Vector alive until the cyclic gc; see gh-559 + # and the matching note in matrix.py. + def _ss(self): + return ss(self) + + _ss.__name__ = _ss.__qualname__ = "ss" + Vector.ss = class_property(property(_ss), ss) else: Vector.ss = class_property( - Vector.ss, 'ss attribute is only available with "suitesparse" backend', exceptional=True + property(), 'ss attribute is only available with "suitesparse" backend', exceptional=True ) diff --git a/graphblas/tests/test_ss_refcount.py b/graphblas/tests/test_ss_refcount.py new file mode 100644 index 000000000..d37a30014 --- /dev/null +++ b/graphblas/tests/test_ss_refcount.py @@ -0,0 +1,144 @@ +"""Regression tests for gh-559: Matrix/Vector must not be born in a reference cycle. + +Historically each Matrix and Vector stored ``self.ss = ss(self)``, and the ss +object (and its config) held ``_parent`` back to the object, so instances died +only via the cyclic garbage collector, not by reference counting. For large +matrices in a tight loop this deferred the release of the C-side GrB buffer +until gc happened to run. ``.ss`` is now built lazily per access and stored +nowhere, so the cycle never forms. These tests pin that behavior; they are +suitesparse-only because ``.ss`` exists only on that backend. +""" + +import gc +import weakref + +import numpy as np +import pytest + +from graphblas import Matrix, Vector, backend, semiring +from graphblas.core.ss.matrix import ss as matrix_ss_class +from graphblas.core.ss.vector import ss as vector_ss_class + +if backend != "suitesparse": + pytest.skip("A.ss only available with suitesparse backend", allow_module_level=True) + + +def _dies_by_reference_count(factory): + """True if the object dies the instant its last strong ref drops, with gc off.""" + gc.collect() + was_enabled = gc.isenabled() + gc.disable() + try: + obj = factory() + wref = weakref.ref(obj) + del obj + return wref() is None + finally: + if was_enabled: + gc.enable() + + +def test_matrix_dies_by_reference_count(): + assert _dies_by_reference_count(lambda: Matrix(float, 5, 5)) + + +def test_vector_dies_by_reference_count(): + assert _dies_by_reference_count(lambda: Vector(float, 5)) + + +def test_mxm_result_dies_by_reference_count(): + A = Matrix.from_dense(np.arange(9.0).reshape(3, 3) + 1) + B = Matrix.from_dense(np.arange(9.0).reshape(3, 3) + 1) + assert _dies_by_reference_count(lambda: A.mxm(B, semiring.min_plus).new()) + + +def test_ss_namespace_is_functional(): + A = Matrix.from_coo([0, 1, 2], [0, 1, 2], [1.0, 2.0, 3.0], nrows=3, ncols=3) + # Built fresh each access (nothing is stored on the instance). + assert A.ss is not A.ss + # Introspection still works through a fresh access. + assert A.ss.nbytes > 0 + matrix_formats = { + "csr", + "csc", + "hypercsr", + "hypercsc", + "bitmapr", + "bitmapc", + "fullr", + "fullc", + "coor", + "cooc", + } + assert A.ss.export()["format"] in matrix_formats + # Config get then set then get, each through an independent `.ss`. + assert A.ss.config["format"] in {"by_row", "by_col"} + A.ss.config["format"] = "by_col" + assert A.ss.config["format"] == "by_col" + + v = Vector.from_coo([0, 2], [1.0, 3.0], size=4) + assert v.ss is not v.ss + assert v.ss.nbytes > 0 + assert v.ss.export()["format"] in {"sparse", "bitmap", "full"} + + +def test_ss_class_access_returns_namespace_class(): + # Class-level access must still yield the ss class so its import_* classmethods work. + assert Matrix.ss is matrix_ss_class + assert Vector.ss is vector_ss_class + assert hasattr(Matrix.ss, "import_any") + assert hasattr(Vector.ss, "import_any") + + +def test_ss_attribute_is_read_only(): + A = Matrix(float, 3, 3) + with pytest.raises(AttributeError): + A.ss = 5 + v = Vector(float, 3) + with pytest.raises(AttributeError): + v.ss = 5 + + +def test_views_have_working_ss(): + # A single-column Matrix cast to a Vector (_as_vector) is a view with _parent set. + A = Matrix.from_coo([0, 1], [0, 0], [1.0, 2.0], nrows=3, ncols=1) + v = A._as_vector() + assert v._parent is A + assert v.ss.nbytes > 0 + # A Vector cast to a Matrix (_as_matrix) is likewise a view. + w = Vector.from_coo([0, 2], [1.0, 3.0], size=4) + M = w._as_matrix() + assert M._parent is w + assert M.ss.nbytes > 0 + + +def test_batched_mxm_loop_does_not_accumulate_matrices(): + # gh-559: with the cyclic collector switched off, a batched + # mxm -> to_dense -> discard loop must not pile up Matrix objects. + rng = np.random.default_rng(0) + A = Matrix.from_dense(rng.random((16, 8)) + 0.1) + B = Matrix.from_dense(rng.random((8, 24)) + 0.1) + + def live_matrices(): + return sum(1 for o in gc.get_objects() if type(o) is Matrix) + + gc.collect() + was_enabled = gc.isenabled() + gc.disable() + try: + # Warm up one iteration so any one-time caches are populated first. + C = Matrix(float, 16, 24) + C << A.mxm(B, semiring.min_plus) + C.to_dense(0.0) + del C + baseline = live_matrices() + for _ in range(50): + C = Matrix(float, 16, 24) + C << A.mxm(B, semiring.min_plus) + C.to_dense(0.0) + del C + # Without the fix this would be baseline + 50. + assert live_matrices() <= baseline + finally: + if was_enabled: + gc.enable()