8000 [PropertyAccess] Major performance improvement by dunglas · Pull Request #16294 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[PropertyAccess] Major performance improvement #16294

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

Closed
wants to merge 8 commits into from
Closed

[PropertyAccess] Major performance improvement #16294

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3ff819e
[PropertyAccess] Major performance improvement
dunglas Oct 20, 2015
fa34c9e
[PropertyAccess] use get_class instead of spl_object_hash
dunglas Oct 20, 2015
b5dabe7
Revert Doctrine Cache and PropertyPath optims
dunglas Oct 20, 2015
de28914
Don't cache dynamic properties.
dunglas Oct 20, 2015
5c16c09
Fix CS
dunglas Oct 20, 2015
8275af1
Fix guess order
dunglas Oct 20, 2015
50d7c0f
Fix CS
dunglas Oct 20, 2015
97d351c
Fix @stof comments
dunglas Oct 25, 2015
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
Prev Previous commit
Next Next commit
Fix guess order
  • Loading branch information
@dunglas
dunglas committed Oct 20, 2015
commit 8275af1ed92b580cdd12376447432fcd00aaf05f
193 changes: 92 additions & 101 deletions src/Symfony/Component/PropertyAccess/PropertyAccessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,17 @@ class PropertyAccessor implements PropertyAccessorInterface
{
const VALUE = 0;
const IS_REF = 1;
const ACCESS_TYPE = 0;
const ACCESS_NAME = 1;
const ACCESS_REF = 2;
const ACCESS_ADDER = 3;
const ACCESS_REMOVER = 4;
const ACCESS_REFLECTION_CLASS = 0;
const ACCESS_TYPE = 1;
const ACCESS_NAME = 2;
const ACCESS_REF = 3;
const ACCESS_ADDER = 4;
const ACCESS_REMOVER = 5;
const ACCESS_TYPE_METHOD = 0;
const ACCESS_TYPE_PROPERTY = 1;
const ACCESS_TYPE_ADDER_AND_REMOVER = 2;
const ACCESS_TYPE_NOT_FOUND = 3;
const ACCESS_TYPE_MAGIC = 2;
const ACCESS_TYPE_ADDER_AND_REMOVER = 3;
const ACCESS_TYPE_NOT_FOUND = 4;

private $magicCall;
private $readPropertyCache = array();
Expand Down Expand Up @@ -218,15 +220,15 @@ private function &readProperty(&$object, $property)
if (isset($this->readPropertyCache[$key])) {
$access = $this->readPropertyCache[$key];
} else {
$access = array();

$access[self::ACCESS_REFLECTION_CLASS] = $reflClass = new \ReflectionClass($object);
Copy link
Member

Choose a reason for hiding this comment

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

putting the ReflectionClass in the cached data would make it much harder to persist the cache across requests. I suggest caching $classHasProperty instead (as this is the only thing you need actually)

$camelProp = $this->camelize($property);
$reflClass = new \ReflectionClass($object);
$getter = 'get'.$camelProp;
$isser = 'is'.$camelProp;
$hasser = 'has'.$camelProp;
$getter = 'get' . $camelProp;
$isser = 'is' . $camelProp;
$hasser = 'has' . $camelProp;
$classHasProperty = $reflClass->hasProperty($property);

$access = array();

if ($reflClass->hasMethod($getter) && $reflClass->getMethod($getter)->isPublic()) {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_METHOD;
$access[self::ACCESS_NAME] = $getter;
Expand All @@ -249,7 +251,7 @@ private function &readProperty(&$object, $property)
$result[self::IS_REF] = true;
} elseif ($this->magicCall && $reflClass->hasMethod('__call') && $reflClass->getMethod('__call')->isPublic()) {
// we call the getter and hope the __call do the job
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_METHOD;
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_MAGIC;
$access[self::ACCESS_NAME] = $getter;
} else {
$methods = array($getter, $isser, $hasser, '__get');
Copy link
Contributor

Choose a reason for hiding this comment

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

Same goes here, it might depend on the instance, and not on the class.

Copy link
Member

Choose a reason for hiding this comment

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

@sescandell there is nothing in this condition depending on $object (except the fact that the case of dynamic properties must have been checked first)

Copy link
Contributor

Choose a reason for hiding this comment

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

You should not cache the not found case.
Imagine the following situation:

$a = new stdClass();
$a->a = 'a';

$b = new stdClass();
$b->b = 'b';
try {
    $accessor->getValue('b', $a);
} catch(...) {...}
try {
    $accessor->getValue('b', $b);
} catch(...) {...}

In that situation it will just throw the exception (except that for $b it should work).

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 will work with your example: https://github.com/dunglas/symfony/blob/propertyaces-perf/src/Symfony/Component/PropertyAccess/PropertyAccessor.php#L284-L293

Only method names are cached but dynamic properties are always tried.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Expand All @@ -259,7 +261,7 @@ private function &readProperty(&$object, $property)

$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_NOT_FOUND;
$access[self::ACCESS_NAME] = sprintf(
'Neither the property "%s" nor one of the methods "%s()" '.
'Neither the property "%s" nor one of the methods "%s()" ' .
'exist and have public access in class "%s".',
$property,
implode('()", "', $methods),
Expand All @@ -270,36 +272,29 @@ private function &readProperty(&$object, $property)
$this->readPropertyCache[$key] = $access;
}

switch ($access[self::ACCESS_TYPE]) {
case self::ACCESS_TYPE_METHOD:
$result[self::VALUE] = $object->{$access[self::ACCESS_NAME]}();
break;

case self::ACCESS_TYPE_PROPERTY:
if ($access[self::ACCESS_REF]) {
$result[self::VALUE] = &$object->{$access[self::ACCESS_NAME]};
$result[self::IS_REF] = true;
} else {
$result[self::VALUE] = $object->{$access[self::ACCESS_NAME]};
}
break;

default:
$reflClass = new \ReflectionClass($object);

if (!$reflClass->hasProperty($property) && property_exists($object, $property)) {
// Needed to support \stdClass instances. We need to explicitly
// exclude $classHasProperty, otherwise if in the previous clause
// a *protected* property was found on the class, property_exists()
// returns true, consequently the following line will result in a
// fatal error.

$result[self::VALUE] = &$object->$property;
$result[self::IS_REF] = true;
} else {
throw new NoSuchPropertyException($access[self::ACCESS_NAME]);
}
break;
if (self::ACCESS_TYPE_METHOD === $access[self::ACCESS_TYPE]) {
$result[self::VALUE] = $object->{$access[self::ACCESS_NAME]}();
} elseif (self::ACCESS_TYPE_PROPERTY === $access[self::ACCESS_TYPE]) {
if ($access[self::ACCESS_REF]) {
$result[self::VALUE] = &$object->{$access[self::ACCESS_NAME]};
$result[self::IS_REF] = true;
} else {
$result[self::VALUE] = $object->{$access[self::ACCESS_NAME]};
}
} elseif (!$access[self::ACCESS_REFLECTION_CLASS]->hasProperty($property) && property_exists($object, $property)) {
// Needed to support \stdClass instances. We need to explicitly
// exclude $classHasProperty, otherwise if in the previous clause
// a *protected* property was found on the class, property_exists()
// returns true, consequently the following line will result in a
// fatal error.

$result[self::VALUE] = &$object->$property;
$result[self::IS_REF] = true;
} elseif (self::ACCESS_TYPE_MAGIC === $access[self::ACCESS_TYPE]) {
// we call the getter and hope the __call do the job
$result[self::VALUE] = $object->{$access[self::ACCESS_NAME]}();
} else {
throw new NoSuchPropertyException($access[self::ACCESS_NAME]);
}

// Objects are always passed around by reference
Expand Down Expand Up @@ -353,11 +348,12 @@ private function writeProperty(&$object, $property, $singular, $value)
$access = $this->writePropertyCache[$key];
} else {
$access = array();
$reflClass = new \ReflectionClass($object);

$access[self::ACCESS_REFLECTION_CLASS] = $reflClass = new \ReflectionClass($object);
$plural = $this->camelize($property);

// Any of the two methods is required, but not yet known
$singulars = null !== $singular ? array($singular) : (array) StringUtil::singularify($plural);
$singulars = null !== $singular ? array($singular) : (array)StringUtil::singularify($plural);

if (is_array($value) || $value instanceof \Traversable) {
$methods = $this->findAdderAndRemover($reflClass, $singulars);
Expand All @@ -367,7 +363,7 @@ private function writeProperty(&$object, $property, $singular, $value)
// message. If the user implements the adder but not the remover,
// an exception will be thrown in findAdderAndRemover() that
// the remover has to be implemented as well.
$guessedAdders = '"add'.implode('()", "add', $singulars).'()", ';
$guessedAdders = '"add' . implode('()", "add', $singulars) . '()", ';
} else {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_ADDER_AND_REMOVER;
$access[self::ACCESS_ADDER] = $methods[0];
Expand All @@ -376,7 +372,7 @@ private function writeProperty(&$object, $property, $singular, $value)
}

if (!isset($access[self::ACCESS_TYPE])) {
$setter = 'set'.$this->camelize($property);
$setter = 'set' . $this->camelize($property);
$classHasProperty = $reflClass->hasProperty($property);

if ($reflClass->hasMethod($setter) && $reflClass->getMethod($setter)->isPublic()) {
Expand All @@ -388,22 +384,14 @@ private function writeProperty(&$object, $property, $singular, $value)
} elseif ($classHasProperty && $reflClass->getProperty($property)->isPublic()) {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_PROPERTY;
$access[self::ACCESS_NAME] = $property;
} elseif (!$classHasProperty && property_exists($object, $property)) {
// Needed to support \stdClass instances. We need to explicitly
// exclude $classHasProperty, otherwise if in the previous clause
// a *protected* property was found on the class, property_exists()
// returns true, consequently the following line will result in a
// fatal error.
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_PROPERTY;
$access[self::ACCESS_NAME] = $property;
} elseif ($this->magicCall && $reflClass->hasMethod('__call') && $reflClass->getMethod('__call')->isPublic()) {
// we call the getter and hope the __call do the job
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_METHOD;
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_MAGIC;
$access[self::ACCESS_NAME] = $setter;
} else {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_NOT_FOUND;
$access[self::ACCESS_NAME] = sprintf(
'Neither the property "%s" nor one of the methods %s"%s()", '.
'Neither the property "%s" nor one of the methods %s"%s()", ' .
'"__set()" or "__call()" exist and have public access in class "%s".',
$property,
$guessedAdders,
Expand All @@ -416,54 +404,57 @@ private function writeProperty(&$object, $property, $singular, $value)
$this->writePropertyCache[$key] = $access;
}

switch ($access[self::ACCESS_TYPE]) {
case self::ACCESS_TYPE_METHOD:
$object->{$access[self::ACCESS_NAME]}($value);
break;

case self::ACCESS_TYPE_PROPERTY:
$object->{$access[self::ACCESS_NAME]} = $value;
break;

case self::ACCESS_TYPE_ADDER_AND_REMOVER:
// At this point the add and remove methods have been found
// Use iterator_to_array() instead of clone in order to prevent side effects
// see https://github.com/symfony/symfony/issues/4670
$itemsToAdd = is_object($value) ? iterator_to_array($value) : $value;
$itemToRemove = array();
$propertyValue = &$this->readProperty($object, $property);
$previousValue = $propertyValue[self::VALUE];
// remove reference to avoid modifications
unset($propertyValue);

if (is_array($previousValue) || $previousValue instanceof \Traversable) {
foreach ($previousValue as $previousItem) {
foreach ($value as $key => $item) {
if ($item === $previousItem) {
// Item found, don't add
unset($itemsToAdd[$key]);

// Next $previousItem
continue 2;
}
if (self::ACCESS_TYPE_METHOD === $access[self::ACCESS_TYPE]) {
$object->{$access[self::ACCESS_NAME]}($value);
} elseif (self::ACCESS_TYPE_PROPERTY === $access[self::ACCESS_TYPE]) {
$object->{$access[self::ACCESS_NAME]} = $value;
} elseif (self::ACCESS_TYPE_ADDER_AND_REMOVER === $access[self::ACCESS_TYPE]) {
// At this point the add and remove methods have been found
// Use iterator_to_array() instead of clone in order to prevent side effects
// see https://github.com/symfony/symfony/issues/4670
$itemsToAdd = is_object($value) ? iterator_to_array($value) : $value;
$itemToRemove = array();
$propertyValue = &$this->readProperty($object, $property);
$previousValue = $propertyValue[self::VALUE];
// remove reference to avoid modifications
unset($propertyValue);

if (is_array($previousValue) || $previousValue instanceof \Traversable) {
foreach ($previousValue as $previousItem) {
foreach ($value as $key => $item) {
if ($item === $previousItem) {
// Item found, don't add
unset($itemsToAdd[$key]);

// Next $previousItem
continue 2;
}

// Item not found, add to remove list
$itemToRemove[] = $previousItem;
}
}

foreach ($itemToRemove as $item) {
call_user_func(array($object, $access[self::ACCESS_REMOVER]), $item);
// Item not found, add to remove list
$itemToRemove[] = $previousItem;
}
}

foreach ($itemsToAdd as $item) {
call_user_func(array($object, $access[self::ACCESS_ADDER]), $item);
}
break;
foreach ($itemToRemove as $item) {
call_user_func(array($object, $access[self::ACCESS_REMOVER]), $item);
}

default:
throw new NoSuchPropertyException($access[self::ACCESS_NAME]);
foreach ($itemsToAdd as $item) {
call_user_func(array($object, $access[self::ACCESS_ADDER]), $item);
}
} elseif (!$access[self::ACCESS_REFLECTION_CLASS]->hasProperty($property) && property_exists($object, $property)) {
// Needed to support \stdClass instances. We need to explicitly
// exclude $classHasProperty, otherwise if in the previous clause
// a *protected* property was found on the class, property_exists()
// returns true, consequently the following line will result in a
// fatal error.

$object->{$access[self::ACCESS_NAME]} = $value;
} elseif (self::ACCESS_TYPE_MAGIC === $access[self::ACCESS_TYPE]) {
$object->{$access[self::ACCESS_NAME]}($value);
} else {
throw new NoSuchPropertyException($access[self::ACCESS_NAME]);
}
}

Expand Down
0