Fix MethodBinding/OverloadMapper memory leak (#691)#2719
Merged
Conversation
MethodBinding and OverloadMapper held PyObject `target` references that were not disposed during tp_clear, leaving Python-side refcount drops to wait on the multi-hop .NET finalizer chain. They also shared the same C# PyObject instance across mp_subscript/Overloads paths, so freeing one could free the underlying Python object out from under the others. - ExtensionType: add virtual OnClear() hook called from tp_clear before the GCHandle is released, letting subclasses eagerly drop owned Python references. - MethodBinding/OverloadMapper: override OnClear to dispose `target`. (`targetType` is intentionally not disposed since Python types are long-lived and tracked by other caches.) - Take an independent INCREF'd PyObject copy at every site that hands a shared target into a new MethodBinding or OverloadMapper, so each wrapper owns its own reference. Result: the three _does_not_leak_memory tests drop from ~485 MB delta to ~10 KB delta on Python 3.14.
The previous 90% threshold (0.9 MB/iter against a 1 MB allocation) documented the issue but did not reproduce it: master leaks ~600-765 KB/iter, which the 0.9 MB threshold accepts as passing. Drop the threshold to 10% (104 KB/iter). On the 2026-05-09 verification run with Python 3.14 GIL on linux-aarch64: Without fix (master): ~572-765 KB/iter (FAIL) With fix (this branch): ~-500 B/iter (PASS) Margin is roughly 6x in either direction across .NET 8 and .NET 10, so the threshold cleanly separates buggy from fixed states without being sensitive to GC noise.
Member
|
If the objects are not shared anymore and are always owned by |
lostmsu
reviewed
May 11, 2026
Contributor
Author
|
Ownership is still explicit: |
- Handle the `PyType` reference in `OverloadMapper` and `MethodBinding` in the same way as the object reference - Unconditionally store the `PyType` of the object - Introduce `NewReference` helper function for the object and type passing - Fix the remaining missing reference count bump for the type (`MethodObject`) - As the count is now correct, `Dispose` the type as well
Member
|
@greateggsgreg I took the liberty of pushing my notes as a commit :)
If these are fine with you, I'd go ahead and merge this once the CI is through. |
Contributor
Author
|
Awesome, looks good. Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #691.
Cause
MethodBindingandOverloadMapperhold aPyObject targetbut didn't release it ontp_clear, so the underlying CLR instance waited on the .NET finalizer chain to drop the refcount. They also shared the same C#PyObjectinstance acrossmp_subscript/Overloadspaths, so disposing one wrapper corrupted the others.Fix
ExtensionType: add virtualOnClear()hook called fromtp_clear.MethodBinding/OverloadMapper: overrideOnClearto disposetarget. (targetTypeleft alone — disposing it broke unrelated subclass tests.)new PyObject(self.target.Reference)so each wrapper owns its own INCREF'd reference.Tests
The three existing
*_does_not_leak_memorytests cover the three sharing sites but their 0.9 MB/iter threshold was too loose — master was leaking ~600 KB/iter and still passing. Tightened to 0.1 MB/iter (104 KB).Verification (Python 3.14 GIL, linux-aarch64)