[#10685] - Unit testing memory management improvements
- Fixed in Code Base
- 22 Aug 2016
- Medium
- Build: staging
- # 10685
- Diff
- photodude:patch-7
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Pull Request for unit testing memory management improvements.
Summary of Changes
- unset() the setup() objects in tearDown()
- add tearDown() where it's missing
- fix missing parent::tearDown();
- add backupServer var for super global where test changes the values (following what was done in other tests)
Testing Instructions
Merge by code review / automated testing review
Before patch (based on recent commit to staging)
| PHP version | Aproximate Run Time | Total Memory use by PHP Unit |
|---|---|---|
| PHP 5.3 | Time: 1.23 minutes | Memory: 352.50Mb |
| PHP 5.4 | Time: 1.39 minutes | Memory: 314.00Mb |
| PHP 5.5 | Time: 1.69 minutes | Memory: 314.50Mb |
| PHP 5.6 | Time: 1.19 minutes | Memory: 314.25Mb |
| PHP 7.0 | Time: 49.44 seconds | Memory: 176.00Mb |
| HHVM | Time: 2.83 minutes | Memory: 271.30MB |
After Patch
| PHP version | Aproximate Run Time | Total Memory use by PHP Unit |
|---|---|---|
| PHP 5.3 | Time: 1.35 minutes | Memory: 271.50Mb |
| PHP 5.4 | Time: 1.3 minutes | Memory: 240.00Mb |
| PHP 5.5 | Time: 1.22 minutes | Memory: 241.25Mb |
| PHP 5.6 | Time: 1.13 minutes | Memory: 240.25Mb |
| PHP 7.0 | Time: 55.05 seconds | Memory: 138.00Mb |
| HHVM | Fatal error: request has exceeded memory limit |
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
?
|
||
| Labels |
Added:
?
?
|
||
| Category | ⇒ | Unit Tests |
| Labels | |||
| Labels | |||
Looks like there's a tradeoff between speed and memory with this PR (gets a little slower but much better on memory, the tests would run again on a 256M PHP memory limit for PHP 5.4+ instead of needing to bump over that). I'd say go for it.
There is probably more room for improvements
After Patch - replacing some = null with unset()
| PHP version | Aproximate Run Time | Total Memory use by PHP Unit |
|---|---|---|
| PHP 5.3 | Time: 1.29 minutes | Memory: 272.00Mb |
| PHP 5.4 | Time: 1.24 minutes | Memory: 240.50Mb |
| PHP 5.5 | Time: 1.22 minutes | Memory: 241.50Mb |
| PHP 5.6 | Time: 1.15 minutes | Memory: 240.50Mb |
| PHP 7.0 | Time: 54.52 seconds | Memory: 138.00Mb |
| HHVM | Fatal error: request has exceeded memory limit |
After Patch - missed an object
| PHP version | Aproximate Run Time | Total Memory use by PHP Unit |
|---|---|---|
| PHP 5.3 | Time: 1.23 minutes | Memory: 271.75Mb |
| PHP 5.4 | Time: 1.22 minutes | Memory: 240.25Mb |
| PHP 5.5 | Time: 1.45 minutes | Memory: 241.25Mb |
| PHP 5.6 | Time: 1.17 minutes | Memory: 240.50Mb |
| PHP 7.0 | Time: 51.20 seconds | Memory: 138.00Mb |
| HHVM | Fatal error: request has exceeded memory limit |
I mean is this really giving us anything?
@wilsonge I attempted this to try to fix the HHVM memory issue. but sadly it didn't work. IMO It's an overall benefit to the testing. Additionally, as @mbabker said
tests would run again on a 256M PHP memory limit for PHP 5.4+ instead of needing to bump over that)
With the last adjustment I did not only is the memory usage better, the times are also now slightly better for most of the cases. (there seems to be some slight variation in timing on each run)
| PHP version | pre patch Run Time | Post Patch runtime |
|---|---|---|
| PHP 5.3 | Time: 1.23 minutes | Time: 1.27 minutes |
| PHP 5.4 | Time: 1.39 minutes | Time: 1.20 minutes |
| PHP 5.5 | Time: 1.69 minutes | Time: 1.51 minutes |
| PHP 5.6 | Time: 1.19 minutes | Time: 1.14 minutes |
| PHP 7.0 | Time: 49.44 seconds | Time: 49.38 seconds |
| HHVM | Time: 2.83 minutes | Memory: 271.30MB |
Is this still a live issue? @photodude
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10685.
| Status | Pending | ⇒ | Information Required |
| Labels | |||
@brianteeman Yes, this is still live.
| Labels | |||
| Status | Information Required | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-08-22 22:13:02 |
| Closed_By | ⇒ | wilsonge |
Honestly I'm still not convinced all these unsets are worth it. But......
| Labels |
Removed:
?
|
||
@mbabker or @wilsonge Do either of you think this PR is worth consideration?