10000 [VarDumper] Keep and reuse array stubs in memory by nicolas-grekas · Pull Request #23683 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 28, 2017
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
[VarDumper] Keep and reuse array stubs in memory
  • Loading branch information
@nicolas-grekas
nicolas-grekas committed Jul 28, 2017
commit 92fa55dd8b59aff66ed4456eba857b99698a93dd
79 changes: 40 additions & 39 deletions src/Symfony/Component/VarDumper/Cloner/VarCloner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@
*/
class VarCloner extends AbstractCloner
{
private static $gid;
private static $hashMask = 0;
private static $hashOffset = 0;
private static $arrayCache = array();

/**
* {@inheritdoc}
Expand All @@ -36,14 +38,15 @@ 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
$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
Copy link
Contributor

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 use random_bytes? (unrelated to the actual change)

Copy link
Member Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

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.

Copy link
Member Author

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 array 10000 s, where users can put any keys. global uniqueness is important

Copy link
Contributor

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.

Copy link
Member Author

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 :)

Copy link
Member Author

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 about ReflectionMethod::getStaticVariables() :)

Copy link
Member Author
@nicolas-grekas nicolas-grekas Jul 27, 2017 •
edited
Loading

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

Copy link
Contributor

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.

self::initHashMask();
}
$gid = self::$gid;
$hashMask = self::$hashMask;
$hashOffset = self::$hashOffset;
$arrayStub = new Stub();
Expand All @@ -58,9 +61,9 @@ protected function doClone($var)
if (\is_int($k)) {
continue;
}
foreach (array($k => true) as $j => $v) {
foreach (array($k => true) as $gk => $gv) {
}
if ($k !== $j) {
if ($gk !== $k) {
$fromObjCast = true;
$refs = $vals = \array_values($queue[$i]);
break;
Expand Down Expand Up @@ -95,7 +98,7 @@ protected function doClone($var)
case true === $v:
case \is_int($v):
case \is_float($v):
break;
continue 2;

case \is_string($v):
if (!\preg_match('//u', $v)) {
Expand All @@ -114,7 +117,10 @@ protected function doClone($var)
$stub->class = Stub::STRING_UTF8;
$stub->cut = $cut;
$stub->value = \mb_substr($v, 0, $maxString, 'UTF-8');
} else {
continue 2;
}
$a = null;
break;

case \is_array($v):
Expand Down Expand Up @@ -146,11 +152,9 @@ protected function doClone($var)
} else {
$a = $v;
}
} else {
} elseif (\PHP_VERSION_ID < 70200) {
$indexedArrays[$len] = true;
}

$stub->value = \count($a);
break;

case \is_object($v):
Expand Down Expand Up @@ -210,42 +214,40 @@ protected function doClone($var)
break;
}

if (isset($stub)) {
if ($a) {
if (!$i || 0 > $maxItems) {
$queue[$len] = $a;
$stub->position = $len++;
} elseif ($pos < $maxItems) {
if ($maxItems < $pos += \count($a)) {
$a = \array_slice($a, 0, $maxItems - $pos);
if ($stub->cut >= 0) {
$stub->cut += $pos - $maxItems;
}
if ($a) {
if (!$i || 0 > $maxItems) {
$queue[$len] = $a;
$stub->position = $len++;
} elseif ($pos < $maxItems) {
if ($maxItems < $pos += \count($a)) {
$a = \array_slice($a, 0, $maxItems - $pos);
if ($stub->cut >= 0) {
$stub->cut += $pos - $maxItems;
}
$queue[$len] = $a;
$stub->position = $len++;
} elseif ($stub->cut >= 0) {
$stub->cut += \count($a);
$stub->position = 0;
}
}

if ($arrayStub === $stub) {
if ($arrayStub->cut) {
$stub = array($arrayStub->cut, $arrayStub->class => $arrayStub->position);
$arrayStub->cut = 0;
} else {
$stub = array($arrayStub->class => $arrayStub->position);
}
$queue[$len] = $a;
$stub->position = $len++;
} elseif ($stub->cut >= 0) {
$stub->cut += \count($a);
$stub->position = 0;
}
} 10000

if ($zvalIsRef) {
$refs[$k]->value = $stub;
if ($arrayStub === $stub) {
if ($arrayStub->cut) {
$stub = array($arrayStub->cut, $arrayStub->class => $arrayStub->position);
$arrayStub->cut = 0;
} elseif (isset(self::$arrayCache[$arrayStub->class][$arrayStub->position])) {
$stub = self::$arrayCache[$arrayStub->class][$arrayStub->position];
} else {
$vals[$k] = $stub;
self::$arrayCache[$arrayStub->class][$arrayStub->position] = $stub = array($arrayStub->class => $arrayStub->position);
}
}

$stub = $a = null;
if ($zvalIsRef) {
$refs[$k]->value = $stub;
} else {
$vals[$k] = $stub;
}
}

Expand All @@ -255,9 +257,9 @@ protected function doClone($var)
$vals = array();
$j = -1;
foreach ($queue[$i] as $k => $v) {
foreach (array($k => true) as $a => $v) {
foreach (array($k => true) as $gk => $gv) {
}
if ($a !== $k) {
if ($gk !== $k) {
$vals = (object) $vals;
$vals->{$k} = $refs[++$j];
$vals = (array) $vals;
Expand All @@ -268,7 +270,6 @@ protected function doClone($var)
}

$queue[$i] = $vals;
unset($indexedArrays[$i]);
Copy link
Member Author

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

Copy link
Contributor

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?

Copy link
Member

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)

Copy link
Member Author

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

Copy link
Member Author

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.

}

foreach ($values as $h => $v) {
Expand Down
0