Skip to content

Alternative attempt at fixing memleak by being explicit about copies #2722

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
filmor wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Alternative attempt at fixing memleak by being explicit about copies #2722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c84d4ab
Alternative attempt at fixing memleak by being explicit about copies
filmor May 11, 2026
3dc93d8
Do not overwrite derived slots from base types
filmor May 12, 2026
5278af4
Simplify slot initialisation further
filmor May 12, 2026
a2c030c
Fix tp_clear for OverloadMapper
filmor May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/runtime/PythonTypes/PyObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ internal PyObject(in StolenReference reference)
Finalizer.Instance.ThrottledCollect();
}

public PyObject Copy() => new(this);
Copy link
Copy Markdown
Member

@lostmsu lostmsu May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this function is too generic and therefore misleading

Copy link
Copy Markdown
Member Author

@filmor filmor May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is it misleading? This is exactly what it does (at least in Python terms). The current behaviour, being able to freely pass the object around, is the misleading thing IMO.


// Ensure that encapsulated Python object is decref'ed appropriately
// when the managed wrapper is garbage-collected.
~PyObject()
Expand Down
2 changes: 2 additions & 0 deletions src/runtime/PythonTypes/PyType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ internal PyType(in StolenReference reference, bool prevalidated = false) : base(
throw new ArgumentException("object is not a type");
}

public new PyType Copy() => new(this);

protected PyType(SerializationInfo info, StreamingContext context) : base(info, context) { }

internal new static PyType? FromNullableReference(BorrowedReference reference)
Expand Down
41 changes: 16 additions & 25 deletions src/runtime/TypeManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ internal class TypeManager
internal static IPythonBaseTypeProvider pythonBaseTypeProvider;
#pragma warning restore CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable.


private const BindingFlags tbFlags = BindingFlags.Public | BindingFlags.Static;
private static readonly Dictionary<MaybeType, PyType> cache = new();

static readonly Dictionary<PyType, SlotsHolder> _slotsHolders = new(PythonReferenceComparer.Instance);
Expand Down Expand Up @@ -654,39 +652,32 @@ static void InheritSubstructs(IntPtr type)
/// </summary>
internal static void InitializeSlots(PyType type, Type impl, SlotsHolder? slotsHolder = null)
{
// We work from the most-derived class up; make sure to get
// the most-derived slot and not to override it with a base
// class's slot.
// FlattenHierarchy provides inherited public static methods for slot lookup.
var seen = new HashSet<string>();
MethodInfo[] methods = impl.GetMethods(BindingFlags.Public | BindingFlags.Static | BindingFlags.FlattenHierarchy);

while (impl != null)
foreach (MethodInfo method in methods)
{
MethodInfo[] methods = impl.GetMethods(tbFlags);
foreach (MethodInfo method in methods)
string name = method.Name;
if (!name.StartsWith("tp_") && !TypeOffset.IsSupportedSlotName(name))
{
string name = method.Name;
if (!name.StartsWith("tp_") && !TypeOffset.IsSupportedSlotName(name))
{
Debug.Assert(!name.Contains("_") || name.StartsWith("_") || method.IsSpecialName);
continue;
}

if (seen.Contains(name))
{
continue;
}

InitializeSlot(type, Interop.GetThunk(method), name, slotsHolder);
Debug.Assert(!name.Contains("_") || name.StartsWith("_") || method.IsSpecialName);
continue;
}

seen.Add(name);
if (seen.Contains(name))
{
continue;
}

var initSlot = impl.GetMethod("InitializeSlots", BindingFlags.Static | BindingFlags.Public);
initSlot?.Invoke(null, parameters: new object?[] { type, seen, slotsHolder });
InitializeSlot(type, Interop.GetThunk(method), name, slotsHolder);

impl = impl.BaseType;
seen.Add(name);
}

var initSlot = impl.GetMethod("InitializeSlots", BindingFlags.Static | BindingFlags.Public | BindingFlags.DeclaredOnly);
initSlot?.Invoke(null, parameters: new object?[] { type, seen, slotsHolder });

SetRequiredSlots(type, seen);
}

Expand Down
23 changes: 16 additions & 7 deletions src/runtime/Types/MethodBinding.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,12 @@ internal class MethodBinding : ExtensionType
internal MaybeMethodInfo info;
internal MethodObject m;
internal PyObject? target;
internal PyType? targetType;
internal PyType targetType;

public MethodBinding(MethodObject m, PyObject? target, PyType? targetType = null)
public MethodBinding(MethodObject m, PyObject? target, PyType targetType)
{
this.target = target;

this.targetType = targetType ?? target?.GetPythonType();

this.targetType = targetType;
this.info = null;
this.m = m;
}
Expand Down Expand Up @@ -54,7 +52,7 @@ public static NewReference mp_subscript(BorrowedReference tp, BorrowedReference
}

MethodObject overloaded = self.m.WithOverloads(overloads);
var mb = new MethodBinding(overloaded, self.target, self.targetType);
var mb = new MethodBinding(overloaded, self.target?.Copy(), self.targetType.Copy());
return mb.Alloc();
}

Expand Down Expand Up @@ -141,7 +139,7 @@ public static NewReference tp_getattro(BorrowedReference ob, BorrowedReference k
// FIXME: deprecate __overloads__ soon...
case "__overloads__":
case "Overloads":
var om = new OverloadMapper(self.m, self.target);
var om = new OverloadMapper(self.m, self.target?.Copy(), self.targetType.Copy());
return om.Alloc();
case "__signature__" when Runtime.InspectModule is not null:
var sig = self.Signature;
Expand Down Expand Up @@ -249,6 +247,17 @@ public static NewReference tp_call(BorrowedReference ob, BorrowedReference args,
}
}

public new static int tp_clear(BorrowedReference ob)
{
var self = (MethodBinding)GetManagedObject(ob)!;

self.target?.Dispose();
self.targetType.Dispose();
self.target = null;

return ExtensionType.tp_clear(ob);
}


/// <summary>
/// MethodBinding __hash__ implementation.
Expand Down
19 changes: 16 additions & 3 deletions src/runtime/Types/OverloadMapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@ namespace Python.Runtime
internal class OverloadMapper : ExtensionType
{
private readonly MethodObject m;
private readonly PyObject? target;
private PyObject? target;
readonly PyType targetType;

public OverloadMapper(MethodObject m, PyObject? target)
public OverloadMapper(MethodObject m, PyObject? target, PyType targetType)
{
this.target = target;
this.targetType = targetType;
this.m = m;
}

Expand Down Expand Up @@ -42,7 +44,7 @@ public static NewReference mp_subscript(BorrowedReference tp, BorrowedReference
return Exceptions.RaiseTypeError(e);
}

var mb = new MethodBinding(self.m, self.target) { info = mi };
var mb = new MethodBinding(self.m, self.target?.Copy(), self.targetType.Copy()) { info = mi };
return mb.Alloc();
}

Expand All @@ -54,5 +56,16 @@ public static NewReference tp_repr(BorrowedReference op)
var self = (OverloadMapper)GetManagedObject(op)!;
return self.m.GetDocString();
}

public new static int tp_clear(BorrowedReference ob)
{
var self = (OverloadMapper)GetManagedObject(ob)!;

self.target?.Dispose();
self.targetType.Dispose();
self.target = null;

return ExtensionType.tp_clear(ob);
}
}
}
6 changes: 3 additions & 3 deletions tests/test_method.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -985,7 +985,7 @@ def test_getting_generic_method_binding_does_not_leak_memory(memory_usage_tracki

# Allow 90% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration
# Increased from 50% to ensure that it works on Windows with Python >3.13
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.9
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.1

assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration

Expand Down Expand Up @@ -1026,7 +1026,7 @@ def test_getting_overloaded_method_binding_does_not_leak_memory(memory_usage_tra
bytesLeakedPerIteration = processBytesDelta / iterations

# Allow 90% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.9
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.1

assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration

Expand Down Expand Up @@ -1069,7 +1069,7 @@ def test_getting_method_overloads_binding_does_not_leak_memory(memory_usage_trac
bytesLeakedPerIteration = processBytesDelta / iterations

# Allow 90% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.9
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.1

assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration

Expand Down
Loading