On Tue, Jan 24, 2012 at 5:58 AM, Trent Nelson <trent_at_snakebite.org> wrote:
>
> On Jan 24, 2012, at 12:11 AM, Daniel Shahaf wrote:
>
>> Trent Nelson wrote on Mon, Jan 23, 2012 at 15:07:20 -0500:
>>> Fix a memory leak in convert_rangelist by *always* Py_DECREF'ing the object
>>> returned by convert_to_swigtype (a SWIG wrapper around svn_merge_range_t)
>>> once we've established it's not NULL.
>>>
>>> This is required because PyList_Append takes ownership of references passed
>>> in (i.e. calls Py_INCREF). Prior to this fix, the reference counts of any
>>
>> Is svn_swig_py_c_strings_to_list() affected too? It appends a string to
>> a list but doesn't Py_DECREF() the string.
>>
>
> Yeah, I saw that, but scheduled it for another day, as I didn't want to clutter my main patch. Also, nothing calls that method -- which also factored into my decision to ignore it for this patch.
>
>>> svn_merge_range_t objects would always be off-by-one, preventing them from
>>> ever being garbage collected, having dire effects on the memory usage of
>>> long-running programs calling svn_mergeinfo_parse() on real-world data.
>>>
>>> Patch by: Trent Nelson <trent_at_snakebite.org>
>>> Tested on: FreeBSD, OS X, Windows.
>>>
>>> * subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c:
>>> (convert_rangelist): Make sure we call Py_DECREF on the object returned
>>> from convert_to_swigtype. PyList_New might return NULL; check for that.
>>> Add 'apr_array_header_t *' and 'swig_type_info *' casts where necessary
>>> to suppress compiler warnings.
>>
>> Those casts shouldn't be needed -- casts from 'void *' to other
>> pointer types are valid C. What warnings were you seeing?
>
> I'll double-check in the morning. Side bar: the bindings generate loads of warnings when compiled with the target platform's equivalent of -Wall.
Yes, this is partly due to the bindings themselves, and partly due to
SWIG. Annoying either way.
> I found a bunch of other things whilst traipsing through the SWIG/Python stuff that I'll send follow up patches for this week. Nothing as bad as the main memory leak, though.
Great! Keep 'em coming!
Though I don't feel qualified to effectively review the patches
(though I suppose I could give it a rough go in a pinch), I am glad
somebody is looking at these problems, and you're making progress.
Thanks for the effort.
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Received on 2012-01-24 10:52:17 CET