Skip to content

Fix all memory leaks#1672

Merged
mvorisek merged 6 commits intodevelopfrom
no_mem_leaks
Oct 7, 2021
Merged

Fix all memory leaks#1672
mvorisek merged 6 commits intodevelopfrom
no_mem_leaks

Conversation

@mvorisek
Copy link
Copy Markdown
Member

@mvorisek mvorisek commented Oct 5, 2021 •
edited
Loading

fixes #1669

All tests must be able to run 1 000 000 times without any memory increase 😄

The following usecases are leaky by design:

minimal leak per iteration, leaks when the array is resized (4x, 8x, 16x, ...)

https://3v4l.org/JAMjY

if (!class_exists(Leaky::class)) {
    class Leaky
    {
        public static $arr = [];
    }
}
Leaky::$arr[] = count(Leaky::$arr);

leaks when php resizes class table (also in multiple of 2, but as many classes are normally already present, takes much more iterations to detect)

https://3v4l.org/jrqso (demo should include the code below in a look and the results depends if opcache is enabled)

new class() {};

@mvorisek mvorisek force-pushed the no_mem_leaks branch 8 times, most recently from 2720a30 to 9aba083 Compare October 5, 2021 23:47
@mvorisek mvorisek mentioned this pull request Oct 5, 2021
Closed
@mvorisek mvorisek force-pushed the no_mem_leaks branch 8 times, most recently from d398b10 to 7061b91 Compare October 6, 2021 16:31
@mvorisek mvorisek force-pushed the no_mem_leaks branch 2 times, most recently from b96303d to 607bf72 Compare October 6, 2021 16:52
@mvorisek mvorisek added the RTM label Oct 6, 2021
@mvorisek mvorisek marked this pull request as ready for review October 6, 2021 17:24
@mvorisek
Copy link
Copy Markdown
Member Author

mvorisek commented Oct 6, 2021 •
edited
Loading

I verified 1600 iterations of unit tests:

PHPUnit 9.5.10 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.0RC3
Configuration: /__w/ui/ui/phpunit.xml.dist

...............................................................  63 / 497 ( 12%)
............................................................... 126 / 497 ( 25%)
.........................F..................................... 189 / 497 ( 38%)
............................................................... 252 / 497 ( 50%)
............................................................... 315 / 497 ( 63%)
............................................................... 378 / 497 ( 76%)
............................................................... 441 / 497 ( 88%)
........................................................        497 / 497 (100%)

You should really speed up these slow tests (>500ms)...
 1. 180392ms to run Atk4\Ui\Tests\JsTest:testNumbers
 2. 46438ms to run Atk4\Ui\Tests\DemosTest:testDemosStatusAndHtmlResponse with data set #59
 3. 46042ms to run Atk4\Ui\Tests\DemosTest:testDemosStatusAndHtmlResponse with data set #49
 4. 41555ms to run Atk4\Ui\Tests\DemosHttpNoExitTest:testDemosStatusAndHtmlResponse with data set #49
 5. 40208ms to run Atk4\Ui\Tests\DemosHttpTest:testDemosStatusAndHtmlResponse with data set #49
 6. 40152ms to run Atk4\Ui\Tests\DemosHttpNoExitTest:testDemosStatusAndHtmlResponse with data set #59
 7. 39672ms to run Atk4\Ui\Tests\DemosHttpTest:testDemosStatusAndHtmlResponse with data set #59
 8. 34168ms to run Atk4\Ui\Tests\DemosHttpNoExitTest:testDemosStatusAndHtmlResponse with data set #28
 9. 32179ms to run Atk4\Ui\Tests\DemosHttpTest:testDemosStatusAndHtmlResponse with data set #80
 10. 31640ms to run Atk4\Ui\Tests\DemosHttpTest:testDemosStatusAndHtmlResponse with data set #105
...and there are 404 more above your threshold hidden from view

Time: 01:25:12.599, Memory: 46.00 MB

There was 1 failure:

1) Atk4\Ui\Tests\DemosTest::testDemosStatusAndHtmlResponse with data set #19 ('basic/button.php')
Memory leak detected! (128.031 + 0.625 + 0.313 KB, 17 iterations)

FAILURES!
Tests: 497, Assertions: 2117919, Failures: 1.

the one failure is leaky experiment correctly detected.

@mvorisek mvorisek merged commit bf817f9 into develop Oct 7, 2021
@mvorisek mvorisek deleted the no_mem_leaks branch October 7, 2021 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DarkSide666 DarkSide666 DarkSide666 approved these changes

@abbadon1334 abbadon1334 Awaiting requested review from abbadon1334

@georgehristov georgehristov Awaiting requested review from georgehristov

@ibelar ibelar Awaiting requested review from ibelar

Assignees

No one assigned

Labels

bug 🪲 RTM

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Something leaks memory

2 participants

@mvorisek @DarkSide666