Do not cache test double declarations #5862
Conversation
sebastianbergmann
added
the
type/performance
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5862 +/- ##
============================================
- Coverage 92.34% 92.34% -0.01%
+ Complexity 6554 6553 -1
============================================
Files 699 699
Lines 19772 19770 -2
============================================
- Hits 18259 18257 -2
Misses 1513 1513 ☔ View full report in Codecov by Sentry. |
| } | ||
|
|
||
| return self::$cache[$key]; | ||
| return $this->generateCodeForTestDoubleClass( |
There was a problem hiding this comment.
I noticed that (counter-intuitively) memory consumption increases when test double code declarations are not cached.
sounds like this method is somehow triggering logic which when invoked repeatedly builds up a buffer or uses a ressource which leaks memory or has side-effects.
let me record a memory diff profile
There was a problem hiding this comment.
when running blackfire on 778070d, I don't see generateCodeForTestDoubleClass at all in the profile -> it's using very little memory and therefore blackfire decides it is not worth having information about this method in the profile.
when running blackfire on this PR, we get a bit more information. generateCodeForTestDoubleClass is still using very little memory, but at least it now shows up in the profile.
in this screen you can see how the memory used by generateCodeForTestDoubleClass spreads over calls triggered from this method:
it looks like the lions share of memory is consumed by reflection and autoloading , which makes sense to me.
both are apis which have side-effects... looking a bit closer it seems the that the caching kills the reflection overhead. autoloading is still visible with a similar amount in the cached variant.
if there is a way to reduce/remove calls to ReflectionClass->getMethods you might be able to remove the caching.
I ran blackfire on the unit test-suite as a whole. in case we could find a cli command in which the test-double generator itself would take a bigger share of the overall memory we could get more precise numbers of the diff between main and this PR (e.g. running phpunit only on a subset of tests which stress the testDouble-Generator more).
| * @psalm-var array<non-empty-string, MockClass> | ||
| */ | ||
| private static array $cache = []; | ||
|
|
||
| /** |
There was a problem hiding this comment.
as I have the numbers at hand atm, maybe this is also interessting (even though not related to this PRs topic):
running the test-suite as a whole, these calls are the most memory consuming ones:
and here you can find the slowest calls:
recorded with
$ php -v
PHP 8.2.12 (cli) (built: Oct 24 2023 21:15:35) (NTS Visual C++ 2019 x64)
Copyright (c) The PHP Group
Zend Engine v4.2.12, Copyright (c) Zend Technologies
with blackfire v1.92.16~win-x64-non_zts82, https://blackfire.io, by Blackfire
While profiling the test double code generator today, I noticed that (counter-intuitively) memory consumption increases when test double code declarations are not cached. For instance, for PHPUnit's own test suite from 48 MB to 52 MB.
This is PR is not intended to be merged, but I would like to have a place where the investigation of this issue can be discussed.