Reduce memory consumption of TypeInferenceTestCase #2644

staabm · 2023-09-24T11:12:20Z

reduce the data beeing transfered between dataProvider and tests, by using just scalar values instead of objects with complex graphs which needs a lot of memory/time to serialize.

refs phpstan/phpstan#9914

repro https://github.com/gnutix/phpstan-typeinferencetest-memory-exhausted

before this PR

➜  memopt git:(main) ✗ vendor/bin/phpunit --no-results

PHPUnit 10.3.5 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.10
Configuration: /Users/staabm/workspace/memopt/phpunit.xml.dist

FFF....FF....FFFFFFFFFFFFFFFFFFFFF........FFF....F.F.....FFFFFF  63 / 168 ( 37%)
F......FFFFF.FFF................FFFFF.....F.....FFFFFFFFF...... 126 / 168 ( 75%)
FFFFFFFFF.......F.....FFFF............FF..                      168 / 168 (100%)

Time: 00:27.074, Memory: 5.38 GB

after this PR

 memopt git:(main) ✗ vendor/bin/phpunit --no-results

PHPUnit 10.3.5 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.10
Configuration: /Users/staabm/workspace/memopt/phpunit.xml.dist

FFF....FF....FFFFFFFFFFFFFFFFFFFFF........FFF....F.F.....FFFFFF  63 / 168 ( 37%)
F......FFFFF.FFF................FFFFF.....F.....FFFFFFFFF...... 126 / 168 ( 75%)
FFFFFFFFF.......F.....FFFF............FF..                      168 / 168 (100%)

Time: 00:00.042, Memory: 82.00 MB

=> ~98% memory improvement

ondrejmirtes · 2023-09-24T14:29:06Z

The time makes me suspicious about this change and the benchmark.

staabm · 2023-09-24T14:44:04Z

The results are similar to the ones sugggested in the initial report of phpstan/phpstan#9914

The blackfire profiles of the report also show a 99% cost of data serialization

staabm · 2023-09-24T18:29:18Z

The time makes me suspicious about this change and the benchmark.

I did more in detail work and found the reason/circumstances when this patch shines:

phpstan/phpstan#9914 (reply in thread)

only when tests are written with global code which gets executed this improvement is necessary.
people should not write tests like this, but I think having this small patch merged to reduce the overhead for this case - because not everyone knows how the best practice looks like - would be a good thing

ondrejmirtes · 2023-09-24T18:54:54Z

Please push your branch with the modified test code (move asserts into closures) so that I can see for myself.

staabm · 2023-09-24T19:04:51Z

see https://github.com/staabm/phpstan-typeinferencetest-memory-exhausted/tree/closures

via enabling this line, you can "enable" the change of this PR

I removed a few test files of the origin repro because it was too massive slow to work with

staabm · 2023-09-25T07:02:02Z

reproducer of the initial reported problem within the PHPStan codebase itself: #2647

in the repro we can also see that the fix of this PR here fixes the problem in phpunit10

ondrejmirtes · 2023-09-25T07:26:46Z

I think we can close this, the root issue was addressed in PHPUnit.

staabm · 2023-09-25T07:40:01Z

agree. upstream fix was in sebastianbergmann/phpunit#5524

staabm · 2023-09-25T12:44:30Z

@gnutix on my laptop I can reproduce that https://github.com/staabm/phpstan-typeinferencetest-memory-exhausted/tree/main makes phpunit slow and hang, while https://github.com/staabm/phpstan-typeinferencetest-memory-exhausted/tree/closures - where I have wrapped the tests in closures - finishes nearly instant using time vendor/bin/phpunit --no-results

mstaab@NB-COMPLEX-61 MINGW64 /c/dvl/Workspace/phpstan-typeinferencetest-memory-exhausted (closures)
$ time vendor/bin/phpunit --no-results
PHPUnit 10.3.5 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.7
Configuration: C:\dvl\Workspace\phpstan-typeinferencetest-memory-exhausted\phpunit.xml.dist

.FFFFFF..F.F....F.FFFFFFFFFFFF                                    30 / 30 (100%)

Time: 00:00.030, Memory: 64.00 MB


real    0m1.236s
user    0m0.000s
sys     0m0.000s

mstaab@NB-COMPLEX-61 MINGW64 /c/dvl/Workspace/phpstan-typeinferencetest-memory-exhausted (main)
$ time vendor/bin/phpunit --no-results
PHPUnit 10.3.5 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.7
Configuration: C:\dvl\Workspace\phpstan-typeinferencetest-memory-exhausted\phpunit.xml.dist

...........F......FFFFFFFFFFFFFFFFF.....F.FFFFF.....FF.........  63 / 168 ( 37%)
...FF.FFFFFFFF.....FFF.FFF..FFF.FFFF.....FF....F.FFFFFFFFFF.... 126 / 168 ( 75%)
.........FFF....FF.FFF........FFFF.....FFF                      168 / 168 (100%)

Time: 00:48.006, Memory: 5.47 GB


real    1m34.071s
user    0m0.000s
sys     0m0.000s

staabm · 2023-09-25T12:54:34Z

ohh hold on a second. I just realized my closure branch also cotained a change which updated phpunit. that might be a red hering.

staabm · 2023-09-25T12:58:53Z

I just force-pushed a new revision into the closure branch and got a different result:

main branch

mstaab@NB-COMPLEX-61 MINGW64 /c/dvl/Workspace/phpstan-typeinferencetest-memory-exhausted (main)

$ time vendor/bin/phpunit --no-results
PHPUnit 10.3.5 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.7
Configuration: C:\dvl\Workspace\phpstan-typeinferencetest-memory-exhausted\phpunit.xml.dist

...........F......FFFFFFFFFFFFFFFFF.....F.FFFFF.....FF.........  63 / 168 ( 37%)
...FF.FFFFFFFF.....FFF.FFF..FFF.FFFF.....FF....F.FFFFFFFFFF.... 126 / 168 ( 75%)
.........FFF....FF.FFF........FFFF.....FFF                      168 / 168 (100%)

Time: 00:43.266, Memory: 5.47 GB


real    1m27.093s
user    0m0.000s
sys     0m0.000s

closure branch: (only 7 of the 31 tests have been wrapped)

$ time vendor/bin/phpunit --no-results
PHPUnit 10.3.5 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.7
Configuration: C:\dvl\Workspace\phpstan-typeinferencetest-memory-exhausted\phpunit.xml.dist

.FFFFFF..F.F....F.FFFFFFFFFFFFFFFFF.....F.FFFFF.....FF.........  63 / 168 ( 37%)
...FF.FFFFFFFF.....FFF.FFF..FFF.FFFF.....FF....F.FFFFFFFFFF.... 126 / 168 ( 75%)
.........FFF....FF.FFF........FFFF.....FFF                      168 / 168 (100%)

Time: 00:34.683, Memory: 4.52 GB


real    1m10.589s
user    0m0.000s
sys     0m0.000s

there still is a big difference, but not as big as I initially reported

gnutix · 2023-09-25T13:33:24Z

OK, that's aligned with what I get locally. For now, I can't use the data providers in my TypeInferenceTest, as I can't yet upgrade to PHPUnit 10.4.x yet (it's still a dev branch, and some dependencies are not yet compatible with it). Even with sebastian/exporter 5.1.1 (sprintf fix), using glob() instead of Symfony's Finder and wrapping all test files in closures, running the full Functional's TypeInferenceTest still results in :

$ php vendor/bin/phpunit -d memory_limit=8G vendor-unmanaged/gammadia/collections/phpstan/tests/Functional/Extensions/TypeInferenceTest.php
PHPUnit 10.3.5 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.7
Configuration: /app/phpunit.xml.dist

...............................................................  63 / 168 ( 37%)
............................................................... 126 / 168 ( 75%)
..........................................                      168 / 168 (100%)

Time: 01:26.282, Memory: 4.06 GB

OK (168 tests, 336 assertions)

So I guess I need to wait for PHPUnit 10.4 to be released and its dependencies to be ready, and then I'll be able to add the data providers again.

staabm · 2023-09-25T13:46:28Z

if not too much work, you might be happy with phpunit9 until then

staabm · 2023-09-25T13:47:44Z

or alternativly you use #2644 in the meantime

gnutix · 2023-09-25T13:49:32Z

if not too much work, you might be happy with phpunit9 until then

I just upgraded everything to PHPUnit 10, so thanks but no thanks! 😆

or alternativly you use #2644 in the meantime

Not sure how I can do that, as PHPStan comes as a PHAR and all. 🤷‍♂️

No biggie, I don't really mind not having the data provider for a few weeks/months.

staabm · 2023-09-25T13:55:37Z

Not sure how I can do that, as PHPStan comes as a PHAR and all. 🤷‍♂️

in my tests I just put the TypeInferenceTestCase.php into a file of the repo and direcly required it from the phpunit autoload file.
that way it will be used and not autoloaded from within the phpstan.phar

but if you can live without it - thats the easiest thing to stay

Reduce memory consumption of TypeInferenceTestCase

e2662c4

clxmstaab force-pushed the memopt branch from 91b6e91 to e2662c4 Compare September 25, 2023 06:54

staabm mentioned this pull request Sep 25, 2023

Reproduce NodeScopeResolverTest memory issue #2647

Closed

staabm closed this Sep 25, 2023

staabm deleted the memopt branch September 25, 2023 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce memory consumption of TypeInferenceTestCase #2644

Reduce memory consumption of TypeInferenceTestCase #2644

staabm commented Sep 24, 2023 •

edited

Loading

ondrejmirtes commented Sep 24, 2023

staabm commented Sep 24, 2023

staabm commented Sep 24, 2023

ondrejmirtes commented Sep 24, 2023

staabm commented Sep 24, 2023 •

edited

Loading

staabm commented Sep 25, 2023 •

edited

Loading

ondrejmirtes commented Sep 25, 2023

staabm commented Sep 25, 2023

staabm commented Sep 25, 2023 •

edited

Loading

staabm commented Sep 25, 2023

staabm commented Sep 25, 2023 •

edited

Loading

gnutix commented Sep 25, 2023 •

edited

Loading

staabm commented Sep 25, 2023

staabm commented Sep 25, 2023

gnutix commented Sep 25, 2023 •

edited

Loading

staabm commented Sep 25, 2023

Reduce memory consumption of TypeInferenceTestCase #2644

Reduce memory consumption of TypeInferenceTestCase #2644

Conversation

staabm commented Sep 24, 2023 • edited Loading

ondrejmirtes commented Sep 24, 2023

staabm commented Sep 24, 2023

staabm commented Sep 24, 2023

ondrejmirtes commented Sep 24, 2023

staabm commented Sep 24, 2023 • edited Loading

staabm commented Sep 25, 2023 • edited Loading

ondrejmirtes commented Sep 25, 2023

staabm commented Sep 25, 2023

staabm commented Sep 25, 2023 • edited Loading

staabm commented Sep 25, 2023

staabm commented Sep 25, 2023 • edited Loading

gnutix commented Sep 25, 2023 • edited Loading

staabm commented Sep 25, 2023

staabm commented Sep 25, 2023

gnutix commented Sep 25, 2023 • edited Loading

staabm commented Sep 25, 2023

staabm commented Sep 24, 2023 •

edited

Loading

staabm commented Sep 24, 2023 •

edited

Loading

staabm commented Sep 25, 2023 •

edited

Loading

staabm commented Sep 25, 2023 •

edited

Loading

staabm commented Sep 25, 2023 •

edited

Loading

gnutix commented Sep 25, 2023 •

edited

Loading

gnutix commented Sep 25, 2023 •

edited

Loading