Own generated native closures and retain their method definitions by 1ndahous3 · Pull Request #8962 · RustPython/RustPython

1ndahous3 · 2026-10-04T14:45:43Z

Summary

Release dynamically generated native closures with their method definitions instead of leaking each allocation.
Keep method definitions alive through descriptor binding and GC clearing, so bound methods retain their callable storage.
Tie callable borrows to the function's lifetime and prevent descriptors from exposing an unowned static method definition.

Extracted from #8944 as an independent fix.

AI assistance

This PR follows our AI policy.

Written with Codex (GPT-6), reviewed by a human before submission.

Summary by CodeRabbit

Bug Fixes
- Native methods and descriptors now retain captured state for as long as it is needed, including after the original definition is dropped.
- Bound methods remain callable when accessed through descriptors, even after intermediate objects are dropped.
- Captured state is released when it is no longer referenced, helping prevent premature loss of state and unnecessary memory retention.

chatgpt-codex-connector · 2026-10-04T14:45:47Z

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.

coderabbitai · 2026-10-04T14:46:34Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml
Review profile: CHILL
Plan: Advanced
Run ID: 624bd0a7-c496-4af4-92a7-ede81fc7985c

📥 Commits

Reviewing files that changed from the base of the PR and between b0c4ae4 and 30064dd.

📒 Files selected for processing: 2

crates/vm/src/builtins/builtin_func.rs
crates/vm/src/builtins/descriptor.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Native method definitions can retain boxed native functions. Descriptor binding passes the method-definition owner to bound functions. Native function references and methods have updated lifetime and traversal behavior.

Changes: Native method lifetime

Layer / File(s)	Summary
Owned method definitions `crates/vm/src/function/method.rs`, `crates/vm/src/vm/context.rs`	`HeapMethodDef` can retain a boxed native function. `Context::new_method_def` uses the owning constructor. A test checks that generated functions and bound methods remain callable after intermediate objects are dropped, and that captured state is released after the final method is dropped.
Native function references `crates/vm/src/builtins/builtin_func.rs`, `crates/vm/src/function/method.rs`	`PyNativeFunction::as_func` returns a reference tied to `self`. `PyNativeMethod` enables traversal and excludes its `class` field from traversal. The bound-function builder accepts and stores an optional method-definition owner.
Descriptor owner binding `crates/vm/src/builtins/descriptor.rs`	Both descriptor types make their `method` field crate-visible. Their `bind` methods pass the optional method-definition owner to bound functions.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: youknowdot

Merge Risk: ⚪ Minimal · up to 30064

The reviewed ownership paths retain dynamically generated closures for bound callables and release them when no longer needed. No actionable merge risk was found; the change is ready for normal checks.

Security architecture risk: 🔵 Low · up to a997f

This change replaces leaked resources with explicit lifetime management while preserving existing access checks. No introduced security defect was established, but cleanup under concurrent or reentrant execution has not been validated end to end.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — The security-relevant scope is in-process native-callable memory safety and resource lifetime. Python callers can invoke existing registered callbacks, but the changed paths do not establish a new capability to select arbitrary native pointers or acquire additional callback authority.

Trust Boundaries and Controls

observed — Existing receiver checks before direct and vectorcall dispatch, and type/subtype checks before classmethod binding, remain unchanged. Existing static/class-method exemptions predate this PR; owner propagation changes storage lifetime, not those validation rules.

Resilience and Maintainability Implications

observed — The collector subtracts only references into its active candidate set. Before clearing, it untracks dead objects and preserves late-resurrected objects and their reachable dependents. Clearing and final drops run within deferred-drop handling, while callable clearing retains the borrowed storage's owner.

Hardening Proposals

proposed — Extend lifetime regression coverage with forced GC, repeated binding, reentrant capture destruction, and concurrent resurrection where threading is enabled. Assert that callable storage remains valid through clearing and is released only after the final owner disappears.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 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: generated native closures own their method definitions and remain retained for safe use.
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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

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.

❤️ Share

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

youknowone · 2026-10-08T08:44:55Z

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

1ndahous3 · 2026-10-08T09:28:56Z

@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.

codspeed · 2026-10-09T14:44:03Z

Merging this PR will improve performance by 76.92%

⚠️

Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

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

Performance Changes

	Benchmark	`BASE`	`HEAD`	Efficiency
⚡	`rustpython[10000]`	14 ms	7.9 ms	+76.92%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.

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

4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

fanninpm · 2026-10-09T14:51:00Z

@codspeedbot explain why performance improved

codspeed · 2026-10-09T14:51:37Z

@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

	`main` (`bd8fdb7`)	PR (`b0c4ae4`)
Total	14 ms	7.9 ms
`GcState::collect_inner` (from `eval_breaker_work`)	6.1 ms (43%)	not present
Compile (`compile_with_syntax_warning_handler`)	5.7 ms	5.7 ms
Parse	~1.0 ms	~1.0 ms

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:

	`main`	PR
`gc.get_count()` at startup	`(818, 3920, 0)`	`(1130, 4618, 0)`
Tracked `method_descriptor`	0	1025
Tracked `classmethod_descriptor`	0	40

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

Summary

Warming up the performance wizard...
Investigate pystone rustpython[10000] improvement — Cause: a GC collection no longer falls inside the measured iteration. Not a real speedup.
Reply with explanation — Reply with explanation
Handling comment — Comment handling finished successfully

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

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

Assisted-by: Codex:gpt-6

Revert method_descriptor and classmethod_descriptor to untracked payloads. Their only object field, _method_def_owner, points to an untracked HeapMethodDef. Restore the derived Traverse for builtin_function_or_method without a clear implementation. default_dealloc calls the clear slot on every deallocation, so the custom clear allocated a Vec for each bound builtin. Use the derived Traverse for builtin_method, skipping the static class. Assisted-by: Claude Code:claude-opus-5-5

youknowone

manual traverse is reverted to fix wrong traversing
Thanks!

youknowone mentioned this pull request Oct 8, 2026

Give builtin_method its own type #9004

Merged

1 of 3 tasks

1ndahous3 and others added 2 commits October 15, 2026 21:01

Own generated native closures and retain their method definitions

053edde

Assisted-by: Codex:gpt-6

youknowone force-pushed the native_function_owner branch from b0c4ae4 to 30064dd Compare October 15, 2026 12:55

youknowone approved these changes Oct 15, 2026

View reviewed changes

youknowone enabled auto-merge (squash) October 15, 2026 12:58

youknowone merged commit a61f37a into RustPython:main Oct 15, 2026
20 checks passed

1ndahous3 deleted the native_function_owner branch October 18, 2026 09:01

Navigation Menu

Repository navigation

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

Own generated native closures and retain their method definitions#8962
youknowone merged 2 commits into
RustPython:mainfrom
1ndahous3:native_function_owner

1ndahous3 commented Oct 4, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

chatgpt-codex-connector Bot commented Oct 4, 2026

Uh oh!

coderabbitai Bot commented Oct 4, 2026 •

edited

Loading

Security review details

Uh oh!

youknowone commented Oct 8, 2026

Uh oh!

1ndahous3 commented Oct 8, 2026

Uh oh!

codspeed Bot commented Oct 9, 2026

Uh oh!

fanninpm commented Oct 9, 2026

Uh oh!

codspeed Bot commented Oct 9, 2026 •

edited

Loading

Uh oh!

youknowone left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

1ndahous3 commented Oct 4, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

AI assistance

Summary by CodeRabbit

Uh oh!

chatgpt-codex-connector Bot commented Oct 4, 2026

Uh oh!

coderabbitai Bot commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security review details

Uh oh!

youknowone commented Oct 8, 2026

Uh oh!

1ndahous3 commented Oct 8, 2026

Uh oh!

codspeed Bot commented Oct 9, 2026

Merging this PR will improve performance by 76.92%

Performance Changes

Footnotes

Uh oh!

fanninpm commented Oct 9, 2026

Uh oh!

codspeed Bot commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

What the flamegraphs show

Why the PR moves the collection

Takeaways

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

1ndahous3 commented Oct 4, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 4, 2026 •

edited

Loading

codspeed Bot commented Oct 9, 2026 •

edited

Loading

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