Twisted test suite requires too much memory

[exarkun]

This is finally getting to the point where it is a practical issue. There are buildslaves which cannot reliable get through the entire test suite without running into allocation failures.

This is a tracking ticket for specific issues which result in an unnecessarily large amount of memory to be allocated by the test suite.

Please add comments related to that topic here.

[radix]

Given that we can't trust that the tests are actually passing now, this needs to be fixed before release.

[glyph]

Can you explain this further? The description of this ticket states that the buildslaves encounter allocation "failures" - the word "failure" here implying to me that the suite fails as a result. Are you saying that we are getting false positives from buildbot on test runs which have actually aborted due to an allocation failure?

[glyph]

(13:09:43) exarkun: so I don't think there's really a release blocking issue here
(13:10:49) exarkun: it's just one new kind of intermittent failure in the test suite

... and I agree, so, removing the milestone.

[therve]

I had a look at this issue. The following diff solves a lot of problems:

Index: twisted/trial/test/test_tests.py
===================================================================
--- twisted/trial/test/test_tests.py    (revision 20638)
+++ twisted/trial/test/test_tests.py    (working copy)
@@ -47,7 +47,7 @@
     def assertSuccessful(self, test, result):
-        self.assertEqual(result.successes,  [(test,)])
+        self.assertEqual(result.successes,  [(str(test),)])
         self.assertEqual(result.failures, [])
         self.assertEqual(result.errors, [])
         self.assertEqual(result.expectedFailures, [])
Index: twisted/trial/runner.py
===================================================================
--- twisted/trial/runner.py     (revision 20638)
+++ twisted/trial/runner.py     (working copy)
@@ -143,10 +143,12 @@
         """
         # we implement this because Python 2.3 unittest defines this code
         # in __call__, whereas 2.4 defines the code in run.
-        for test in self._tests:
+        while self._tests:
+            test = self._tests.pop(0)
             if result.shouldStop:
                 break
             test(result)
+            del test
         return result
Index: twisted/trial/reporter.py
===================================================================
--- twisted/trial/reporter.py   (revision 20638)
+++ twisted/trial/reporter.py   (working copy)
@@ -144,7 +144,7 @@
         @type test: L{pyunit.TestCase}
         """
-        self.successes.append((test,))
+        self.successes.append((str(test),))
     def upDownError(self, method, error, warn, printStatus):
         pass

Is that an acceptable solution? I just fixed the existing tests, but I can add new ones, especially for the TestSuite change.

[exarkun]

I thought about making a similar change recently. I think the only real problem with that approach is that it breaks -u (I think). Somebody needs to take responsibility for re-initializing self._tests if the suite is run again.

[therve]

Yes, you're right, I hadn't spot that (there's probably a test missing somewhere, because the trial suite pass).

The problem if you re-initialize self._tests is that you change the semantic of until failure. Maybe that's not so different, I don't know. We can also use a different suite for until-failure. After all, it's not used on the buildbots.

[therve]

(In [20752]) Add 2 optimizations, fix the tests.

Refs #2275

[therve]

With the above commit, trial twisted.conch takes 35M of memory instead of 137M before.

[therve]

(In [20761]) Add a few tests.

Refs #2275

[therve]

I think that's clean enough to review in trial-memory-2275.

[radix]

[1] The str(test) change is really weird. I looked into it and I don't even see why you should be keeping any of these as test cases *or* strings. Why not just use a counter?

[2] The whitebox test seems insufficient to me; I suggest instead writing a test that grabs a weakref to the TestCase? instance during its run and then after trial has a chance to clean it up, call gc.collect() and make sure the weakref is invalid.

Thanks for the work!

[therve]

(In [20875]) Change successes to be a counter.

Refs #2275.

[therve]

(In [20876]) Add a blackbox test on the collection of test intances.

Refs #2275

[therve]

Thanks for the inputs, it should be OK.

[itamarst]

The other categories of test results (failures, errors) can also cause memory leaks; arguably a lot more in the case of errors and failures if the traceback doesn't get converted to string immediately, which I think is what happens now.

So why not, instead of storing test instances and then rendering them with a formatter method, store the result of calling the formatter method? In virtually all cases that should use less memory.

[therve]

Replying to itamarst:

So why not, instead of storing test instances and then rendering them with a formatter method, store the result of calling the formatter method? In virtually all cases that should use less memory.

The problem with that approach is that you limit what is made available to the reporter at the end: if you want further informations about the test that fail, you need the full test object, not just a string representation. Of course for now we don't use that, so that may not matter. My other point is that you don't save many things with that, since there are much more successes than failures (in practice).

[itamarst]

The formatters on the reporter would be called as the tests run, so presumably different reporters would have different formatters?

But it's true there's (usually) less failures than successes, at least in the buildbot scenario.

[therve]

Oh you're right, a Reporter is also a TestResult?, so that works. I'm working on it.

[therve]

Hum some tests like test_tests.TestSkipMethods.test_reasons suppose that it got the full test object in skips, so that actually doesn't work. But I fixed some errors due to the change to successes in the process.

[jml]

Thanks for doing this.

This branch really makes two independent changes: storing 'successes' as a counter rather than a list, and changing to use CleanupTestSuite. It would be easier to review if the changes were in separate branches.

• Please rename CleanupTestSuite to DestructiveTestSuite. Trial already uses the word "cleanup" too much, and it should be painfully obvious that this suite destroys data.
• I really dislike using assertTrue and assertFalse in CleanupTestSuiteTestCase.test_cleanup. Use the countTestCases public method instead.
• That you added TestSuiteUsed.test_untilFailureSuite worries me. Did no other test fail before you added the condition in twisted/scripts/trial.py?

Other than that, the branch looks good. Please make changes / answer questions and assign back to me.

[jml]

Also, it would be very good to have some measurements of memory usage so we can figure out how effective this branch is.

[therve]

(In [21055])

• Rename CleanupTestSuite? to DestructiveTestSuite?
• Clean a test

Refs #2275

[therve]

Replying to jml:

Thanks for doing this. This branch really makes two independent changes: storing 'successes' as a counter rather than a list, and changing to use CleanupTestSuite. It would be easier to review if the changes were in separate branches.

Yep sorry. The changes were intended to be small, but it always grows up.

* Please rename CleanupTestSuite to DestructiveTestSuite. Trial already uses the word "cleanup" too much, and it should be painfully obvious that this suite destroys data.

Done

* I really dislike using assertTrue and assertFalse in CleanupTestSuiteTestCase.test_cleanup. Use the countTestCases public method instead.

Done.

* That you added TestSuiteUsed.test_untilFailureSuite worries me. Did no other test fail before you added the condition in twisted/scripts/trial.py?

Nope :/. I didn't find any other clean way to do that.

[therve]

Replying to jml:

Also, it would be very good to have some measurements of memory usage so we can figure out how effective this branch is.

OK, here are some stats, for some suites:

twisted.web:

• before: VmPeak: 39668 kB, VmSize: 38584 kB
• after: VmPeak: 30652 kB, VmSize: 28756 kB

twisted.conch:

• before: VmPeak: 139204 kB, VmSize:139204 kB
• after: VmPeak: 18944 kB, VmSize: 18944 kB

twisted.test:

• before: VmPeak: 163504 kB, VmSize: 118104 kB
• after: VmPeak: 147488 kB, VmSize: 100540 kB

I didn't make a run on whole suite, but I guess it's there where the improvement is the best.

[exarkun]

Just a couple things:

• TestResult?.successes is undocumented
• I wonder if we care about how this changes the behavior of countTestCases - returning an ever diminishing number over the course of a run seems like it might be bad. trial itself only ever calls it once, at the beginning of a run, though.

[exarkun]

Hrm. I guess one other thing. I'm not sure the single unit test for DestructiveTestSuite? is really sufficient. The only thing it verifies is that there are no test cases left at the end of the run. There's no verification that the implementation actually runs any of the tests, for example.

[therve]

(In [21113])

• Add doc for successes
• Add a small test

Refs #2275

[therve]

I think the behavior of countTestCases is sane, and adjusted other things.

[exarkun]

The shouldStop check in the loop is still untested. But... as far as I can tell, nothing in trial ever sets shouldStop to anything. Maybe just delete that code? The rest looks great.

[therve]

(In [21137])

• Add a test for shouldStop

Refs #2275

[therve]

shouldStop is used by stdlib TestResult, so it's used in trial. I added a test for it.

[exarkun]

Looks good, please merge :)

[therve]

(In [21138]) Merge trial-memory-2275

Author: therve Reviewers: jml, exarkun Fixes #2275

Add two optmizations to trial to reduce its memory footprint: add a new DestructiveTestSuite?, used by default, that don't keep a reference to test instances after run, and replace the storage of successfull result by a counter. This allows the garbage collector to clean a lot of objects, and also speed up the whole suite.