Own generated native closures and retain their method definitions - #8962

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

No actionable comments were generated in the recent review. 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 9.52% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: owning generated native closures and retaining their method definitions.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

---

---

• Fix all pre-merge checks with AI

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Thank you for contributing. about this patch, I investigated the topic and then I found #9004 approach looks better at the moment.

@youknowone following #9004, I propose keeping the new type model and narrowing this change to dynamic callable lifetimes. Context::new_method_def still uses Box::leak, so captured state is never released. HeapMethodDef could own the closure, with bound callables retaining the definition’s owner so cleanup cannot leave dangling references.

If you support this focused ownership fix, I can prepare a new PR or remake the current.

Merging this PR will improve performance by 76.92%

⚡ 1 improved benchmark
✅ 61 untouched benchmarks
⏩ 4 skipped benchmarks¹

Performance Changes

_{Comparing 1ndahous3:native_function_owner (b0c4ae4) with main (bd8fdb7)}

@codspeedbot explain why performance improved

@codspeedbot explain why performance improved

Why pystone::rustpython[10000] "improved" by 77%

This is not a real speedup of pystone. The PR changes when the automatic cyclic GC runs. On main, a full GC collection happened inside the measured iteration of rustpython[10000]. With this PR, it no longer does.

What the flamegraphs show

Everything except the GC pass costs the same. The ~6 ms difference is one collect_inner call: list/dict/tuple traversal via gc_extend_referent_ptrs, plus dropping the garbage it found. This also explains the odd baseline on main. There, rustpython[10000] (14 ms) is slower than rustpython[20000] and rustpython[30000] (7.9 / 7.8 ms), even though it runs fewer loops. The first pystone case was the one paying for the collection.

Why the PR moves the collection

The PR switches PyMethodDescriptor and PyClassMethodDescriptor to traverse = "manual". A payload with HAS_TRAVERSE gets GC-tracked when it's allocated (crates/vm/src/object/core.rs), so these descriptors are now tracked. Before, they weren't. I built main and this PR and checked a fresh interpreter:

Each interpreter in benches/execution.rs now starts with ~1,000 more tracked objects. Generation counts are shared across the whole process, and the gen0 threshold is 2000, so the point where collections trigger moves. The collection that used to land in pystone's single measured iteration now runs outside the measured window (for example during interpreter setup). None of the other 61 benchmarks changed.

Takeaways

• Treat the +77% as a side effect of the benchmark setup, not a win from this PR. Any change in how many objects get tracked can move this result either way.
• The descriptors are now tracked on purpose: they hold _method_def_owner and need to be traversed. The real cost is small: each collection has ~1k more objects to scan.
• To stop this benchmark from flipping on unrelated changes, the harness could call gc.collect() (or disable automatic GC) before the measured loop in bench_rustpython_code, so no collection falls inside the timed section.

Flamegraphs: main · PR · comparison

_{Learnings updated. See what CodSpeed remembers about this repository.}

_{Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.}