BUG: Memory Leak in _GenericBinaryOutFunction #7482
Conversation
|
Oh given this bug is a memory leak (and a small one at that) I'm not entirely sure how to write a regression test for this. Any ideas? |
simongibbons
force-pushed
the
clip_memory_leak_fix
branch
from
f8cb11f
to
ea0d1b3
Compare
| if (kw == NULL) { | ||
| Py_DECREF(args); | ||
| return NULL; | ||
| kw = PyDict_New(); |
There was a problem hiding this comment.
Why not build the dictionary with Py_BuildValue and spare ourselves all the trouble?
kw = Py_BuildValue("{s:s}", "casting", "unsafe");
|
Yup you're right - that really does simplify things. I'm not sure why I thought |
|
It is not very explicit in the prose of the documentation, which mostly discusses single objects vs tuples, but it lets you build single objects, tuples |
|
Right, looks like the build is passing with that change. Like I said a test for this is tricky - if you're ok without putting one in for this then I think this may be ready for merging. It just needs me to squash the commits (and clean up the fairly horrendous english in the commit message - really should have proof read that!) |
|
I would like to know if using But it probably makes sense making that dict |
simongibbons
force-pushed
the
clip_memory_leak_fix
branch
from
9b9444e
to
4b508ea
Compare
|
The string interning is not really the cause of the leak, it is just the cause of the reference leak being visible as a memory leak in some python versions and not in others. |
_GenericBinaryOutFunction was leaking a string each time it was called. This fixes the leak by storing the (constant) **kwargs that are passed into the op statically so that they are only constructed the first time that _GenericBinaryOutFunction is called. Credit to @seberg for finding the location of the bug and providing an initial version of the fix.
simongibbons
force-pushed
the
clip_memory_leak_fix
branch
from
4b508ea
to
321d7ef
Compare
|
@seberg Is more needed here? |
|
No, I think this is fine (tests are probably annoying to figure out). Thanks for the ping and thanks @simongibbons. |
_GenericBinaryOutFunction was leaking constructing a string
each time it was called which wasn't deallocated on some
versions of python (due to string interning rules). This
fixes this by storing the **kwargs that are passed into
the op statically so that they are only constructed
the first time that _GenericBinaryOutFunction is called.
Credit to @seberg for finding the location of the
bug and providing an initial version of the fix.
Fixes #6672