[VarDumper] Keep and reuse array stubs in memory #23683

nicolas-grekas · Jul 26, 2017

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)

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~~ 216MB

https://blackfire.io/profiles/compare/846b58bc-7863-4502-9ca2-f82eebd4173f/graph

nicolas-grekas · Jul 26, 2017

On a smaller profile with GC disabled to be sure we don't compare just GC's time:
https://blackfire.io/profiles/compare/53798f9a-e2ab-42b4-94b4-a564f15814d5/graph
still -32% in memory, and -5% in wall time.

Another https://blackfire.io achievement! :)

Koc · Jul 26, 2017

18.6s is too much anyway

nicolas-grekas · Jul 26, 2017

@Koc see #23659 for the core reason why this test page is slow.

…1598) - added local cache for a logger collector - overwrite symfony`s data_collector.logger class - added tests

kelunik · Jul 26, 2017

src/Symfony/Component/VarDumper/Cloner/VarCloner.php

        $a = null;                      // Array cast for nested structures
        $stub = null;                   // Stub capturing the main properties of an original item value
                                        // 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


Any reason why this doesn't just use random_bytes? (unrelated to the actual change)

Just history, since there is a version of this code that works on PHP 5.3 and the provided uniqueness is good enough

PHP 5.3 is not 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 the uniqid that auto-triggered me writing a comment.

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.

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

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.

you can just read the current value with [...]

true, fixed now :)

Hum, not quite, I forgot about ReflectionMethod::getStaticVariables() :)

~~now replaced by a local variable, using spl_object_hash instead of uniqid~~

Right, there's no real way to prevent that. It's not public API, if people break it, they deserve what they get.

nicolas-grekas · Jul 27, 2017

src/Symfony/Component/VarDumper/Cloner/VarCloner.php

@@ -268,7 +273,6 @@ protected function doClone($var)
            }

            $queue[$i] = $vals;
-            unset($indexedArrays[$i]);


another µ-optim: unsetting scalars from arrays does not free memory, but takes CPU, better not doing it

Curious: Why does it not free memory?

because they are still referenced elsewhere (in the variable being cloned)

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

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.

fabpot · Jul 28, 2017

src/Symfony/Component/VarDumper/Cloner/VarCloner.php

@@ -36,7 +37,7 @@ protected function doClone($var)
        $maxItems = $this->maxItems;
        $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


Not sure if that's relevant in this context, but spl_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.

you're right, back to my first version, it was fine :)

fabpot · Jul 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~~ 216MB https://blackfire.io/profiles/compare/846b58bc-7863-4502-9ca2-f82eebd4173f/graph Commits ------- 92fa55d [VarDumper] Keep and reuse array stubs in memory

nicolas-grekas added this to the 3.3 milestone Jul 26, 2017

carsonbot added Status: Needs Review DependencyInjection Bug labels Jul 26, 2017

nicolas-grekas changed the title ~~[DI] Keep and reuse array stubs in memory~~ [VarDumper] Keep and reuse array stubs in memory Jul 26, 2017

nicolas-grekas added VarDumper and removed DependencyInjection labels Jul 26, 2017

nicolas-grekas force-pushed the var-perf branch from 7f94d7e to d78100a Compare July 26, 2017 13:38

vtsykun referenced this pull request in oroinc/platform Jul 26, 2017

BAP-14658: LoggerDataCollector called over 500k times in dev mode (#1…

915723a

…1598) - added local cache for a logger collector - overwrite symfony`s data_collector.logger class - added tests

kelunik reviewed Jul 26, 2017

View reviewed changes

nicolas-grekas force-pushed the var-perf branch from d78100a to 63d783d Compare July 27, 2017 11:21

nicolas-grekas commented Jul 27, 2017

View reviewed changes

nicolas-grekas force-pushed the var-perf branch 2 times, most recently from c1dcf06 to d63887b Compare July 27, 2017 12:20

fabpot reviewed Jul 28, 2017

View reviewed changes

nicolas-grekas force-pushed the var-perf branch from d63887b to a57fcb4 Compare July 28, 2017 06:05

[VarDumper] Keep and reuse array stubs in memory

92fa55d

nicolas-grekas force-pushed the var-perf branch from a57fcb4 to 92fa55d Compare July 28, 2017 06:06

fabpot approved these changes Jul 28, 2017

View reviewed changes

fabpot merged commit 92fa55d into symfony:3.3 Jul 28, 2017

nicolas-grekas deleted the var-perf branch July 28, 2017 16:42

fabpot mentioned this pull request Aug 1, 2017

Release v3.3.6 #23738

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

[VarDumper] Keep and reuse array stubs in memory #23683

[VarDumper] Keep and reuse array stubs in memory #23683

Uh oh!

Conversation

nicolas-grekas commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Jul 26, 2017

Uh oh!

Koc commented Jul 26, 2017

Uh oh!

nicolas-grekas commented Jul 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jul 28, 2017

Uh oh!

Uh oh!

nicolas-grekas commented Jul 26, 2017 •

edited

Loading

nicolas-grekas Jul 27, 2017 •

edited

Loading