Ticket #2275 (closed defect: fixed )

Opened 2 years ago

Last modified 1 year ago

Twisted test suite requires too much memory

Reported by: exarkun Assigned to: therve
Type: defect Priority: highest
Milestone: Component: core
Keywords: tests Cc: jml, radix, itamarst, exarkun, spiv
Branch: Author:
Launchpad Bug:

Description

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.

Attachments

Change History

  2006-12-07 00:41:07+00:00 changed by jml

  • cc set to jml
  • keywords set to tests

  2006-12-09 19:57:04+00:00 changed by radix

  • milestone set to Twisted-2.5

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

  2006-12-10 17:48:43+00:00 changed by 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?

  2006-12-10 18:33:39+00:00 changed by glyph

  • milestone deleted
(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.

  2007-07-16 13:34:46+00:00 changed by therve

  • owner changed from glyph to therve
  • priority changed from normal to high

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.

  2007-07-16 13:36:48+00:00 changed by 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.

  2007-07-16 14:13:55+00:00 changed by 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.

  2007-07-17 09:54:01+00:00 changed by therve

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

Refs #2275

  2007-07-17 09:58:26+00:00 changed by therve

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

  2007-07-17 15:08:26+00:00 changed by therve

(In [20761]) Add a few tests.

Refs #2275

  2007-07-17 15:09:51+00:00 changed by therve

  • keywords changed from tests to tests review
  • owner deleted
  • priority changed from high to highest

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

  2007-07-29 22:29:42+00:00 changed by radix

  • keywords changed from tests review to tests
  • owner set to therve

[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!

  2007-07-30 08:31:03+00:00 changed by therve

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

Refs #2275.

  2007-07-30 08:41:56+00:00 changed by therve

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

Refs #2275

  2007-07-30 08:43:04+00:00 changed by therve

  • keywords changed from tests to tests review
  • owner changed from therve to radix

Thanks for the inputs, it should be OK.

follow-up: ↓ 17   2007-08-01 12:41:34+00:00 changed by itamarst

  • cc changed from jml to jml, radix, itamarst
  • keywords changed from tests review to tests
  • owner changed from radix to therve

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.

in reply to: ↑ 16   2007-08-01 13:00:53+00:00 changed by 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).

  2007-08-01 13:46:10+00:00 changed by 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.

  2007-08-01 14:26:01+00:00 changed by therve

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

  2007-08-01 15:29:31+00:00 changed by therve

  • keywords changed from tests to tests review

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.

  2007-08-01 15:32:55+00:00 changed by therve

  • owner deleted

follow-up: ↓ 25   2007-08-12 03:14:08+00:00 changed by jml

  • keywords changed from tests review to tests
  • owner set to therve

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.

follow-up: ↓ 26   2007-08-12 03:54:35+00:00 changed by jml

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

  2007-08-13 08:23:19+00:00 changed by therve

(In [21055])

Refs #2275

in reply to: ↑ 22   2007-08-13 08:28:33+00:00 changed by therve

  • keywords changed from tests to tests review
  • owner changed from therve to jml

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.

in reply to: ↑ 23   2007-08-13 09:17:33+00:00 changed by 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.

  2007-08-19 19:15:11+00:00 changed by exarkun

  • cc changed from jml, radix, itamarst to jml, radix, itamarst, exarkun
  • keywords changed from tests review to tests
  • owner changed from jml to therve

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.

  2007-08-19 19:29:04+00:00 changed by 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.

  2007-08-20 08:12:13+00:00 changed by therve

(In [21113])

  • Add doc for successes
  • Add a small test

Refs #2275

  2007-08-20 08:49:19+00:00 changed by therve

  • keywords changed from tests to tests review
  • owner deleted

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

  2007-08-20 13:02:31+00:00 changed by spiv

  • cc changed from jml, radix, itamarst, exarkun to jml, radix, itamarst, exarkun, cc

  2007-08-20 13:03:05+00:00 changed by spiv

  • cc changed from jml, radix, itamarst, exarkun, cc to jml, radix, itamarst, exarkun, spiv

  2007-08-21 13:19:08+00:00 changed by exarkun

  • keywords changed from tests review to tests
  • owner set to therve

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.

  2007-08-21 14:33:23+00:00 changed by therve

(In [21137])

  • Add a test for shouldStop

Refs #2275

  2007-08-21 14:39:05+00:00 changed by therve

  • keywords changed from tests to tests review

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

  2007-08-21 15:15:44+00:00 changed by therve

  • owner deleted

  2007-08-21 16:28:47+00:00 changed by exarkun

  • keywords changed from tests review to tests
  • owner set to therve

  2007-08-21 16:36:06+00:00 changed by exarkun

Looks good, please merge :)

  2007-08-21 16:42:46+00:00 changed by therve

  • status changed from new to closed
  • resolution set to fixed

(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.

Note: See TracTickets for help on using tickets.