[Performance] GridView shim's get_grid re-allocates a Grid wrapper + weakref on every call (100% cache miss) #348

New issue

Closed

opened 2026-07-10 23:47:50 +00:00 by john · 0 comments

john commented

2026-07-10 23:47:50 +00:00

Owner

Summary

Discovered via Callgrind (the #345 profiling rig), not from reading structure — this is an emergent cost of the #252 GridView shim.

mcrfpy.Grid is a GridView shim over GridData. Internal code that needs the underlying UIGrid calls UIGridView::get_grid (src/UIGridView.cpp). That function:

Reconstructs an aliasing shared_ptr<UIGrid> from grid_data.
Looks up PythonObjectCache by serial number.
On miss: tp_allocs a temporary PyUIGridObject wrapper, allocates a weakref to it via PyWeakref_NewRef, and registerObjects that weakref in the cache.
Returns the temporary.

The caller uses the wrapper and drops it → the wrapper is freed → the cached weakref immediately dies → the next get_grid call misses and re-allocates the entire wrapper + weakref + hashtable entry. The cache can never hit because it only ever holds a weakref to a short-lived temporary.

Evidence (Callgrind, full-loop workload)

get_grid called 3,600× in the workload; callee counts inside it:

Callee	Count	Ir
`PythonObjectCache::lookup`	3,600×	486,926
`PyWeakref_NewRef`	3,600×	802,811
`PythonObjectCache::registerObject`	3,600×	1,310,400

Lookups == registrations == weakref allocations == call count → 0% cache hit rate. Inclusive cost ~3.55M Ir (~987 Ir/call), all of it wrapper alloc + weakref alloc + hashtable insert/erase churn, thrown away each call.

Impact

Hits any code that derives the grid repeatedly through the shim:

Per-cell map generation / procgen (a 200×200 fill = ~40,000 wasted alloc cycles)
Entity cell_pos access
Layer writes touching the parent grid

Invisible from project structure — none of #342/#343/#344 would surface it. Same class of fix as #331: stop re-doing per-call ceremony on a hot path.

Proposed fix (direction)

Avoid reconstructing a throwaway wrapper per call. Options:

Cache a strong reference to the shim's own UIGrid wrapper (own it for the shim's lifetime) instead of a weakref-to-temporary, or
Store the PyObject* wrapper on GridData/UIGrid and return it (INCREF) without re-allocating, or
Have internal callers acquire the grid wrapper once and reuse it across a loop rather than re-deriving per cell.

Watch the ownership cycle (Grid ↔ wrapper) — a strong ref needs a break path (the existing tp_dealloc use_count() guard pattern, #251).

Benchmark

Reproduce with a per-cell loop (grid.at() / layer set() over a large grid) under make callgrind; compare get_grid inclusive Ir and registerObject/PyWeakref_NewRef call counts before/after. Cache-hit path should drop weakref allocations to ~1.

Notes

Found while profiling with the new make profile + Callgrind workflow (#345).
Methodology caveat noted separately: forced-render profiling via automation.screenshot() spends ~96% of instructions in PNG encoding (libsfml stbi_zlib_compress) — profile update/animation with step()-only to avoid that.

Related: #331, #345, #252 (GridView shim), #251 (dealloc ownership guard).

## Summary Discovered via Callgrind (the #345 profiling rig), **not** from reading structure — this is an emergent cost of the #252 GridView shim. `mcrfpy.Grid` is a GridView shim over `GridData`. Internal code that needs the underlying `UIGrid` calls `UIGridView::get_grid` (`src/UIGridView.cpp`). That function: 1. Reconstructs an aliasing `shared_ptr<UIGrid>` from `grid_data`. 2. Looks up `PythonObjectCache` by serial number. 3. On miss: `tp_alloc`s a **temporary** `PyUIGridObject` wrapper, allocates a **weakref** to it via `PyWeakref_NewRef`, and `registerObject`s that weakref in the cache. 4. Returns the temporary. The caller uses the wrapper and drops it → the wrapper is freed → **the cached weakref immediately dies** → the next `get_grid` call misses and re-allocates the entire wrapper + weakref + hashtable entry. The cache can never hit because it only ever holds a weakref to a short-lived temporary. ## Evidence (Callgrind, full-loop workload) `get_grid` called **3,600×** in the workload; callee counts inside it: | Callee | Count | Ir | |---|---|---| | `PythonObjectCache::lookup` | 3,600× | 486,926 | | `PyWeakref_NewRef` | 3,600× | 802,811 | | `PythonObjectCache::registerObject` | 3,600× | 1,310,400 | Lookups == registrations == weakref allocations == call count → **0% cache hit rate**. Inclusive cost ~3.55M Ir (~987 Ir/call), all of it wrapper alloc + weakref alloc + hashtable insert/erase churn, thrown away each call. ## Impact Hits any code that derives the grid repeatedly through the shim: - Per-cell map generation / procgen (a 200×200 fill = ~40,000 wasted alloc cycles) - Entity `cell_pos` access - Layer writes touching the parent grid Invisible from project structure — none of #342/#343/#344 would surface it. Same *class* of fix as #331: stop re-doing per-call ceremony on a hot path. ## Proposed fix (direction) Avoid reconstructing a throwaway wrapper per call. Options: - Cache a **strong** reference to the shim's own `UIGrid` wrapper (own it for the shim's lifetime) instead of a weakref-to-temporary, or - Store the `PyObject*` wrapper on `GridData`/`UIGrid` and return it (INCREF) without re-allocating, or - Have internal callers acquire the grid wrapper once and reuse it across a loop rather than re-deriving per cell. Watch the ownership cycle (Grid ↔ wrapper) — a strong ref needs a break path (the existing tp_dealloc `use_count()` guard pattern, #251). ## Benchmark Reproduce with a per-cell loop (`grid.at()` / layer `set()` over a large grid) under `make callgrind`; compare `get_grid` inclusive Ir and `registerObject`/`PyWeakref_NewRef` call counts before/after. Cache-hit path should drop weakref allocations to ~1. ## Notes - Found while profiling with the new `make profile` + Callgrind workflow (#345). - Methodology caveat noted separately: forced-render profiling via `automation.screenshot()` spends ~96% of instructions in PNG encoding (libsfml `stbi_zlib_compress`) — profile update/animation with `step()`-only to avoid that. Related: #331, #345, #252 (GridView shim), #251 (dealloc ownership guard).