Opened 8 years ago

Closed 7 years ago

#2275 defect closed fixed (fixed)

Twisted test suite requires too much memory

Reported by: exarkun Owned by:
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.

Change History (40)

comment:1 Changed 8 years ago by jml

  • Cc jml added
  • Keywords tests added

comment:2 Changed 8 years ago 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.

comment:3 Changed 8 years ago 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?

comment:4 Changed 8 years ago by glyph

  • Milestone Twisted-2.5 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.

comment:5 Changed 7 years ago 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.

comment:6 Changed 7 years ago 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.

comment:7 Changed 7 years ago 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.

comment:8 Changed 7 years ago by therve

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

Refs #2275

comment:9 Changed 7 years ago by therve

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

comment:10 Changed 7 years ago by therve

(In [20761]) Add a few tests.

Refs #2275

comment:11 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted
  • Priority changed from high to highest

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

comment:12 Changed 7 years ago by radix

  • Keywords review removed
  • 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!

comment:13 Changed 7 years ago by therve

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

Refs #2275.

comment:14 Changed 7 years ago by therve

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

Refs #2275

comment:15 Changed 7 years ago by therve

  • Keywords review added
  • Owner changed from therve to radix

Thanks for the inputs, it should be OK.

comment:16 follow-up: Changed 7 years ago by itamarst

  • Cc radix itamarst added
  • Keywords review removed
  • 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.

comment:17 in reply to: ↑ 16 Changed 7 years ago 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).

comment:18 Changed 7 years ago 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.

comment:19 Changed 7 years ago by therve

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

comment:20 Changed 7 years ago by therve

  • Keywords review added

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.

comment:21 Changed 7 years ago by therve

  • Owner therve deleted

comment:22 follow-up: Changed 7 years ago by jml

  • Keywords review removed
  • 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.

comment:23 follow-up: Changed 7 years ago 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.

comment:24 Changed 7 years ago by therve

(In [21055])

  • Rename CleanupTestSuite to DestructiveTestSuite
  • Clean a test

Refs #2275

comment:25 in reply to: ↑ 22 Changed 7 years ago by therve

  • Keywords review added
  • 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.

comment:26 in reply to: ↑ 23 Changed 7 years ago 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.

comment:27 Changed 7 years ago by exarkun

  • Cc exarkun added
  • Keywords review removed
  • 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.

comment:28 Changed 7 years ago 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.

comment:29 Changed 7 years ago by therve

(In [21113])

  • Add doc for successes
  • Add a small test

Refs #2275

comment:30 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

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

comment:31 Changed 7 years ago by spiv

  • Cc cc added

comment:32 Changed 7 years ago by spiv

  • Cc spiv added; cc removed

comment:33 Changed 7 years ago by exarkun

  • Keywords review removed
  • 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.

comment:34 Changed 7 years ago by therve

(In [21137])

  • Add a test for shouldStop

Refs #2275

comment:35 Changed 7 years ago by therve

  • Keywords review added

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

comment:36 Changed 7 years ago by therve

  • Owner therve deleted

comment:37 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

comment:38 Changed 7 years ago by exarkun

Looks good, please merge :)

comment:39 Changed 7 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:40 Changed 4 years ago by <automation>

  • Owner therve deleted
Note: See TracTickets for help on using tickets.