[Bugfix] mcrfpy.get_metrics() leaks 8 allocator blocks per call (PyDict_SetItemString does not steal) #413

New issue

Open

opened 2026-07-27 18:24:12 +00:00 by john · 0 comments

john commented

2026-07-27 18:24:12 +00:00

Owner

Found while calibrating the allocator gate for the #395 soak test, and verified independently.

Measured

build/mcrogueface --headless, 2000-call warmup, gc.collect() before and after, 20000 calls:

mcrfpy.get_metrics()               N= 20000  blocks 59393 -> 219395  growth= 160002  per-call=8.0001
frame.x = float (control)          N= 20000  blocks 219394 -> 219396  growth=      2  per-call=0.0001
entity.cell_pos read (control)     N= 20000  blocks 219395 -> 219397  growth=      2  per-call=0.0001

Exactly 8 sys.getallocatedblocks() per call, perfectly linear, with clean controls either side. Never reclaimed.

The defect

McRFPy_API::_getMetrics (src/McRFPy_API.cpp):

PyDict_SetItemString(dict, "frame_time",     PyFloat_FromDouble(game->metrics.frameTime));
PyDict_SetItemString(dict, "avg_frame_time", PyFloat_FromDouble(game->metrics.avgFrameTime));
PyDict_SetItemString(dict, "fps",            PyLong_FromLong(game->metrics.fps));
...

PyDict_SetItemString does not steal a reference — it increfs the value. Every PyFloat_FromDouble / PyLong_FromLong here is a new reference that is never released, so each one leaks. It is 8 rather than 17 because the small-int cache serves the values in [-5, 256] from singletons, so leaking those costs a refcount but no allocator block; the floats and the out-of-cache ints are real allocations. (That also means the rate varies with the values: a long-running process with current_frame > 256 leaks more per call than a fresh one.)

Two return values are also unchecked for NULL, and PyDict_SetItemString's own return is discarded throughout.

Why it matters more than it looks

get_metrics() is the natural thing to call once per frame from a monitor, a HUD, or a test harness. At 60 fps that is ~1.7 million leaked objects per hour. Several tests already call it in a loop — tests/integration/observe_under_frame_lock_test.py does — and it had to be kept out of the measured region of tests/integration/frame_lock_soak_test.py precisely because it would have swamped that file's allocator gate.

Fix

Either hold each value in a temporary and Py_DECREF after the set, or build the dict with Py_BuildValue("{s:d, s:d, s:i, ...}") which handles ownership. Check the returns while there. A regression gate belongs with it: sys.getallocatedblocks() across N calls, gated well under 1 block per call, in the style of tests/integration/frame_lock_soak_test.py section 4 — RSS is far too coarse to see this.

Sibling: #414 (entity.cell_pos assignment leaks 1 block). Related: #395.

Found while calibrating the allocator gate for the #395 soak test, and verified independently. ## Measured `build/mcrogueface --headless`, 2000-call warmup, `gc.collect()` before and after, 20000 calls: ``` mcrfpy.get_metrics() N= 20000 blocks 59393 -> 219395 growth= 160002 per-call=8.0001 frame.x = float (control) N= 20000 blocks 219394 -> 219396 growth= 2 per-call=0.0001 entity.cell_pos read (control) N= 20000 blocks 219395 -> 219397 growth= 2 per-call=0.0001 ``` Exactly 8 `sys.getallocatedblocks()` per call, perfectly linear, with clean controls either side. Never reclaimed. ## The defect `McRFPy_API::_getMetrics` (`src/McRFPy_API.cpp`): ```cpp PyDict_SetItemString(dict, "frame_time", PyFloat_FromDouble(game->metrics.frameTime)); PyDict_SetItemString(dict, "avg_frame_time", PyFloat_FromDouble(game->metrics.avgFrameTime)); PyDict_SetItemString(dict, "fps", PyLong_FromLong(game->metrics.fps)); ... ``` **`PyDict_SetItemString` does not steal a reference** — it increfs the value. Every `PyFloat_FromDouble` / `PyLong_FromLong` here is a new reference that is never released, so each one leaks. It is 8 rather than 17 because the small-int cache serves the values in [-5, 256] from singletons, so leaking those costs a refcount but no allocator block; the floats and the out-of-cache ints are real allocations. (That also means the *rate* varies with the values: a long-running process with `current_frame > 256` leaks more per call than a fresh one.) Two return values are also unchecked for `NULL`, and `PyDict_SetItemString`'s own return is discarded throughout. ## Why it matters more than it looks `get_metrics()` is the natural thing to call once per frame from a monitor, a HUD, or a test harness. At 60 fps that is ~1.7 million leaked objects per hour. Several tests already call it in a loop — `tests/integration/observe_under_frame_lock_test.py` does — and it had to be kept *out of* the measured region of `tests/integration/frame_lock_soak_test.py` precisely because it would have swamped that file's allocator gate. ## Fix Either hold each value in a temporary and `Py_DECREF` after the set, or build the dict with `Py_BuildValue("{s:d, s:d, s:i, ...}")` which handles ownership. Check the returns while there. A regression gate belongs with it: `sys.getallocatedblocks()` across N calls, gated well under 1 block per call, in the style of `tests/integration/frame_lock_soak_test.py` section 4 — RSS is far too coarse to see this. Sibling: #414 (`entity.cell_pos` assignment leaks 1 block). Related: #395.