Trent Nelson wrote on Tue, Jan 24, 2012 at 10:17:24 -0500:
> On Tue, Jan 24, 2012 at 03:44:04AM -0800, Daniel Shahaf wrote:
> > Trent Nelson wrote on Mon, Jan 23, 2012 at 15:07:20 -0500:
> > > Index: subversion/bindings/swig/python/tests/mergeinfo.py
> > > ===================================================================
> > > --- subversion/bindings/swig/python/tests/mergeinfo.py (revision 1234786)
> > > +++ subversion/bindings/swig/python/tests/mergeinfo.py (working copy)
> > > + def test_mergeinfo_leakage__lingering_range_t_objects_after_del(self):
> > > + """Ensure that there are no svn_merge_range_t objects being tracked by
> > > + the garbage collector after we explicitly `del` the results returned
> > > + by svn_mergeinfo_parse(). We disable gc before the svn_mergeinfo_parse
> > > + call and force an explicit collection cycle straight after the `del`;
> > > + if our reference counts are correct, the allocated svn_merge_range_t
> > > + objects will be garbage collected and thus, not appear in the list of
> > > + objects returned by gc.get_objects()."""
> > > + gc.disable()
> > > + mergeinfo = core.svn_mergeinfo_parse(self.TEXT_MERGEINFO1)
> > > + del mergeinfo
> > > + gc.collect()
> >
> > It seems you need a gc.enable() call as well, to prevent the test from
> > having effects on global state.
>
> You know what, you can get rid of that gc.disable() call. I put it
> in by habit; when I was trying to find the leak, I had breakpoints
> set within the CPython garbage collection routines -- if I didn't
> disable automatic collection, they'd trigger at extremely annoying
> times ;-)
>
> The gc.collect() call is sufficient.
>
Okay. Given that, and that it seems a good idea to add a regression
test for a bug that can cost many MB of memory if it repeats, I've
committed the testsuite part too: r1235296, and will nominate for
backport.
> > Do the swig-py tests support parallelized running? The testsuite for
> > 'svn' does, and if the testsuite for swig-py does too then the 'gc' call
> > have effects on other threads.
>
> I have... no idea :-) Given that check-swig-py basically just calls
> `python subversion/bindings/swig/python/tests/run_all.py', and that
> file just uses the standard Python unit test facilities... I'd say
> no, they're not currently designed to support parallel running.
>
> I'm sure we could totally shave that 40s test run time down a few
> notches if they, though ;-)
>
I think the ROI will be better on optimizing the test suite of 'svn'
itself (build/run_tests.py and subversion/tests/cmdline/) :-)
> Trent.
>
Thanks,
Daniel
Received on 2012-01-24 16:34:03 CET