Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[VarDumper] Keep and reuse array stubs in memory#23683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedJul 26, 2017
On a smaller profile with GC disabled to be sure we don't compare just GC's time: Anotherhttps://blackfire.io achievement! :) |
Koc commentedJul 26, 2017
18.6s is too much anyway |
nicolas-grekas commentedJul 26, 2017
…1598)- added local cache for a logger collector- overwrite symfony`s data_collector.logger class- added tests
| // or null if the original value is used directly | ||
| if (!self::$hashMask) { | ||
| self::$gid =uniqid(mt_rand(),true);// Unique string used to detect the special $GLOBALS variable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Any reason why this doesn't just userandom_bytes? (unrelated to the actual change)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just history, since there is a version of this code that works on PHP 5.3 and the provided uniqueness is good enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
PHP 5.3 isnot a problem, but if the provided uniqueness is good enough, that's fine, too. Didn't look what it's used for, it's just theuniqid that auto-triggered me writing a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If it's just a unique string, why not just use a global counter with$counter = "a"; $counter++? This ensures uniqueness in the process and avoids two function calls there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it's a unique string that needs to be collision free with the $GLOBALS arrays, where users can put any keys. global uniqueness is important
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If you want to, you can just read the current value with(function () { var_dump(self::$gid); })->bindTo(null, VarCloner::class)();, but I think I gave enough input, don't care to discuss it further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you can just read the current value with [...]
true, fixed now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hum, not quite, I forgot aboutReflectionMethod::getStaticVariables() :)
nicolas-grekasJul 27, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
now replaced by a local variable, using spl_object_hash instead of uniqid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Right, there's no real way to prevent that. It's not public API, if people break it, they deserve what they get.
| } | ||
| $queue[$i] =$vals; | ||
| unset($indexedArrays[$i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
another µ-optim: unsetting scalars from arrays does not free memory, but takes CPU, better not doing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Curious: Why does it not free memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
because they are still referenced elsewhere (in the variable being cloned)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
well, not exactly for this reason in this case: it's because PHP allocates arrays as they grow, and never frees the allocated memory when they shrink. I think this article might explain it:http://jpauli.github.io/2016/04/08/hashtables.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So, when the table is full and when we empty it, the memory usage doesn't move at all (modulo noise). When we finished unset()ing every value, we end having a hashtable which arData is 32768 slots all full of UNDEF zvals.
c1dcf06 tod63887bCompare| $maxString =$this->maxString; | ||
| $cookie = (object)array();// Unique object used to detect hard references | ||
| $gid =uniqid(mt_rand(),true);// Unique string used to detect the special $GLOBALS variable | ||
| $gid =\spl_object_hash($cookie);// Unique string used to detect the special $GLOBALS variable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure if that's relevant in this context, butspl_object_hash does not guarantee that it returns a unique value. The value can be reused when objects are reclaimed. And we did hit that limitation in the past (and Doctrine as well), so that's not just theoretical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you're right, back to my first version, it was fine :)
fabpot commentedJul 28, 2017
Thank you@nicolas-grekas. |
…grekas)This PR was merged into the 3.3 branch.Discussion----------[VarDumper] Keep and reuse array stubs in memory| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -(to be reviewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/23683/files?w=1))As highlighted by@vtsykun in#23620, the patch in#23644 improves performance but increases the memory usage.The issue is that even a small `array($k => $v)` consumes more memory than a `new Stub()`.That's a shame, but since our small arrays have a low variety, we can keep them in a cache and leverage php's COW mechanism to reduce memory. Effectively, this creates a memory leak. But the leak is upper bounded by the data you had already in memory, since that's what is cloned. Looking at the numbers, it looks worth it:| | 3.3.5 | +#23644 | +this PR| --- | --- | --- | ---| Wall Time | 39.4s | 26.1s | ~~18.6s~~ 17.3s| Memory | 391MB | 539MB | ~~217MB~~ 216MBhttps://blackfire.io/profiles/compare/846b58bc-7863-4502-9ca2-f82eebd4173f/graphCommits-------92fa55d [VarDumper] Keep and reuse array stubs in memory
Uh oh!
There was an error while loading.Please reload this page.
(to be reviewedignoring whitespaces)
As highlighted by@vtsykun in#23620, the patch in#23644 improves performance but increases the memory usage.
The issue is that even a small
array($k => $v)consumes more memory than anew Stub().That's a shame, but since our small arrays have a low variety, we can keep them in a cache and leverage php's COW mechanism to reduce memory. Effectively, this creates a memory leak. But the leak is upper bounded by the data you had already in memory, since that's what is cloned. Looking at the numbers, it looks worth it:
18.6s17.3s217MB216MBhttps://blackfire.io/profiles/compare/846b58bc-7863-4502-9ca2-f82eebd4173f/graph