Use private properties to store sensible and big variables to improve memory usage (Enhancement)

[Aerendir]

I'm refactoring some of my test class to use less memory as I had some problems with large memory usage on my CI server.

Going deeper in the code to find issues I came up with a trait that can teardown automatically the tests and set all properties to null to free up memory space.

The trait also provides some useful methods to better manage internal variables and resources, storing them in some properties that are then set to null.

This bundle seems to use a lot of memory as it creates a lot of very big variables.

It may be sufficient to use properties to store objects like kernel, clients, and similar thing, and, adding a simple tearDown() method, set those to null to free up memory space and improve tests performance.

Maybe this helpful to you?

[lsmith77]

yeah .. I think the biggest memory hog is the container etc .. which I think leaks memory .. sure send a PR!

[Aerendir]

Perfect! I'll work on this :)

[tackerm]

Yesss, +1 on improving memory usage!

You may want to take a look at this: http://kriswallsmith.net/post/18029585104/faster-phpunit
We do something similar in our tests, and it helps a lot!

It also helps to force a garbage collection cycle in the tearDown() method (however, if you are using the phpunit-bridge note that the bridge currently disables garbage collection).

[alexislefebvre]

IIRC, this Bundle inherits from Symfony test class, is the tearDown() method from Symfony implements this optimisation?

[tackerm]

It just shuts down the kernel in KernelTestCase::tearDown(), but does not clean up anything else.

[alexislefebvre]

Maybe it would be interesting to submit the same patch to Symfony?

[tackerm]

Yes and no.
In terms of clean coding, every class should take care of cleaning up after itself. So every *Test-class should have a tearDown() method that removes/unsets all the properties created by that class.

The Symfony test classes do not set any properties (except for the kernel, which is also being shut down). The Liip\FunctionalTestBundle\Test\WebTestCase class however should take care of properly unsetting the $environment, $containers, $kernelDir, $maxMemory, $verbosityLevel, $decorated properties.

The patch mentioned above is basically just a shortcut for the lazy developer - I'm not sure if this should be provided/encouraged by the framework :)

[Aerendir]

@alexislefebvre , Maybe... The WebTestCase class should have a tearDown() method and all extending classes should have their own for their own properties and call the parent::tearDown() method... So WebTestCaseTest should have its own tearDown() and call WebTestCase::tearDown().

[Aerendir]

@tackerm , yes, you are right. Using reflection I found an average o 5 Kb increase in memory usage for smaller tests. For bigger ones, instead, the average memory freed is about 30Kb... But this, obviously, depends on the kind of test, the number of variables, ecc...

Anyway I agree with you about: each class should take care of tear down itself... The reflection is a temporary workaround, not a definitive solution...

[Aerendir]

I think this is a very big task, so may be better to split it into smaller tasks, as one for the $client, one for $kernel, one for $container, and so on.
This way it will be easier also to maintain PRs aligned with master branch, as this refactoring will change practically each single method in the class.

More, with the occasion, may be useful to also move one level up some methods, to reduce the number of lines in the WebTestCase file. To start (and avoid BC), we may create a top level TestCase file, where to move some common methods and extend WebTestCase from this.

What do you think about? This kind of reorganization may be also helpful to complete issue #6... Anyway in the next days I will better examine the situation and provide a more detailed list of possible tasks.

[tackerm]

Just sharing some early-morning-thoughts:

• I think a simple tearDown() method where the class-properties are being unset would help a lot in terms of improved memory usage.
• Note that switching to private properties (as the title suggests) would probably be a huge BC break, since extended classes can no longer access properties that have been protected so far. Also, I am not sure if such a change would have a big impact on memory usage.
• Refactoring the entire class structure would definitely be useful for future improvements, but I think it is not necessary to solve this particular task (unless I am missing something?). Maybe this should be a milestone for a 2.0 release though... :)

[lsmith77]

yeah a refactoring would be great to do for 2.0. see also he oldest open ticket #6

[Aerendir]

@lsmith77 , yes, I saw it and linked too! :) I agree, it may be very useful.
@tackerm , in sparse order:

• Yes for the tearDown(): it's exactly as said;
• The refactoring isn't necessary to solve this particular issue, sure, but with the occasion may be the preparation for the future versions. But, about this point some guidance by @lsmith77 is needed;
• Switching to private properties: yes, it is a huge BC break. But in some places (I haven't seen deeply all the code, but in getVerbosityLevel() and in getDecorated() is possible) it is possible to trigger some trigger_error('', E_USER_DEPRECATED) intercepting the direct access to the property. I think is a good choice to use getters and setters and that using them will also simplify the code. But, again, guidance on this by @lsmith77 are very necessary. Also because, again, these changes will have a big impact on the code itself and old PRs will be more difficult to re-align.

[alexislefebvre]

The setUp() and tearDown() template methods are run once for each test method (and on fresh instances) of the test case class.

In addition, the setUpBeforeClass() and tearDownAfterClass() template methods are called before the first test of the test case class is run and after the last test of the test case class is run, respectively.

Source: https://phpunit.de/manual/current/en/fixtures.html

The tearDownAfterClass() method may be used to clean data after the last test of the class. What do you think?

[Aerendir]

I think that the first step to take is to start using properties instead of local variables. Then we'll can (and I'll can too, as at the moment I'm still understanding the code) decide if the best choice is a setUp()/tearDown() couple or it's better to use *afterClass() methods.

Take the $kernel variable: I'm sure this decision is affected by the decision of reusing or not the already instantiated kernel... Only to name one possible case... I think a deeper understanding of the code is needed, at least by me, to make a good decision.