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

Show file tree
Hide file tree
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
Next Next commit
[PropertyAccess] Major performance improvement
  • Loading branch information
@dunglas
dunglas committed Oct 20, 2015
commit 3ff819e7a1d0ae69a0ea152dc4b446c00a8d1ab8
301 changes: 205 additions & 96 deletions src/Symfony/Component/PropertyAccess/PropertyAccessor.php
View file
Open in desktop
10000
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\PropertyAccess;

use Doctrine\Common\Cache\Cache;
use Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException;
use Symfony\Component\PropertyAccess\Exception\UnexpectedTypeException;

Expand All @@ -23,27 +24,38 @@ 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_TYPE_METHOD = 0;
const ACCESS_TYPE_PROPERTY = 1;
const ACCESS_TYPE_ADDER_AND_REMOVER = 2;
const ACCESS_TYPE_NOT_FOUND = 3;

private $magicCall;
private $cache;
private $propertyPathCache = array();
private $readPropertyCache = array();
private $writePropertyCache = array();

/**
* Should not be used by application code. Use
* {@link PropertyAccess::createPropertyAccessor()} instead.
*/
public function __construct($magicCall = false)
public function __construct($magicCall = false, Cache $cache = null)
Copy link
Member

Choose a reason for hiding this comment

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

This is a BC break, because we already have a second argument in newer branches, and it is not a Cache instance

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like setter injection but it seems to be the way to go to keep BC. What do you think?

{
$this->magicCall = $magicCall;
$this->cache = $cache;
}

/**
* {@inheritdoc}
*/
public function getValue($objectOrArray, $propertyPath)
{
if (!$propertyPath instanceof PropertyPathInterface) {
$propertyPath = new PropertyPath($propertyPath);
}

$propertyPath = $this->getPropertyPath($propertyPath);
$propertyValues = &$this->readPropertiesUntil($objectOrArray, $propertyPath, $propertyPath->getLength());

return $propertyValues[count($propertyValues) - 1][self::VALUE];
Expand All @@ -54,10 +66,7 @@ public function getValue($objectOrArray, $propertyPath)
*/
public function setValue(&$objectOrArray, $propertyPath, $value)
{
if (!$propertyPath instanceof PropertyPathInterface) {
$propertyPath = new PropertyPath($propertyPath);
}

$propertyPath = $this->getPropertyPath($propertyPath);
$propertyValues = &$this->readPropertiesUntil($objectOrArray, $propertyPath, $propertyPath->getLength() - 1);
$overwrite = true;

Expand Down Expand Up @@ -202,48 +211,92 @@ private function &readProperty(&$object, $property)
throw new NoSuchPropertyException(sprintf('Cannot read property "%s" from an array. Maybe you should write the property path as "[%s]" instead?', $property, $property));
}

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

if ($reflClass->hasMethod($getter) && $reflClass->getMethod($getter)->isPublic()) {
$result[self::VALUE] = $object->$getter();
} elseif ($reflClass->hasMethod($isser) && $reflClass->getMethod($isser)->isPublic()) {
$result[self::VALUE] = $object->$isser();
} elseif ($reflClass->hasMethod($hasser) && $reflClass->getMethod($hasser)->isPublic()) {
$result[self::VALUE] = $object->$hasser();
} elseif ($reflClass->hasMethod('__get') && $reflClass->getMethod('__get')->isPublic()) {
$result[self::VALUE] = $object->$property;
} elseif ($classHasProperty && $reflClass->getProperty($property)->isPublic()) {
$result[self::VALUE] = &$object->$property;
$result[self::IS_REF] = true;
} 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.
$result[self::VALUE] = &$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
$result[self::VALUE] = $object->$getter();
} else {
$methods = array($getter, $isser, $hasser, '__get');
if ($this->magicCall) {
$methods[] = '__call';
$key = spl_object_hash($object).'::'.$property;

if (isset($this->readPropertyCache[$key])) {
$access = $this->readPropertyCache[$key];
} elseif (!$this->cache || !$access = $this->cache->fetch('read'.$key)) {
Copy link
Member

Choose a reason for hiding this comment

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

persisting the cache across requests does not make any sense, as your cache key relies on spl_object_hash, which is not shared across requests

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, using the class fully qualified name should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it actually caching values per instance::property or per class::property? Because the first is not really a case you can cover across requests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it's by instance (because of spl_object_hash) but I suggest to change it by class (it will also improve performances when different objects of the same type are serialized).

$camelProp = $this->camelize($property);
$reflClass = new \ReflectionClass($object);
$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;
} elseif ($reflClass->hasMethod($isser) && $reflClass->getMethod($isser)->isPublic()) {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_METHOD;
$access[self::ACCESS_NAME] = $isser;
} elseif ($reflClass->hasMethod($hasser) && $reflClass->getMethod($hasser)->isPublic()) {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_METHOD;
$access[self::ACCESS_NAME] = $hasser;
} elseif ($reflClass->hasMethod('__get') && $reflClass->getMethod('__get')->isPublic()) {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_PROPERTY;
$access[self::ACCESS_NAME] = $property;
$access[self::ACCESS_REF] = false;
} elseif ($classHasProperty && $reflClass->getProperty($property)->isPublic()) {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_PROPERTY;
$access[self::ACCESS_NAME] = $property;
$access[self::ACCESS_REF] = true;

$result[self::VALUE] = &$object->$property;
$result[self::IS_REF] = true;
} elseif (!$classHasProperty && property_exists($object, $property)) {
Copy link
Member

Choose a reason for hiding this comment

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

This cannot be cached, as it depends on the instance, not on the class

// 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;
$access[self::ACCESS_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;
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong. You need a separate access type for magic methods, because you must check the dynamic properties before calling the magic method (otherwise it changes the current behavior)

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

👍

if ($this->magicCall) {
$methods[] = '__call';
}

$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_NOT_FOUND;
$access[self::ACCESS_NAME] = sprintf(
'Neither the property "%s" nor one of the methods "%s()" ' .
'exist and have public access in class "%s".',
$property,
implode('()", "', $methods),
$reflClass->name
);
}

throw new NoSuchPropertyException(sprintf(
'Neither the property "%s" nor one of the methods "%s()" '.
'exist and have public access in class "%s".',
$property,
implode('()", "', $methods),
$reflClass->name
));
$this->readPropertyCache[$key] = $access;
if ($this->cache) {
$this->cache->save('read'.$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:
throw new NoSuchPropertyException($access[self::ACCESS_NAME]);
}

// Objects are always passed around by reference
Expand Down Expand Up @@ -291,16 +344,88 @@ private function writeProperty(&$object, $property, $singular, $value)
throw new NoSuchPropertyException(sprintf('Cannot write property "%s" to an array. Maybe you should write the property path as "[%s]" instead?', $property, $property));
}

$reflClass = new \ReflectionClass($object);
$plural = $this->camelize($property);
$key = spl_object_hash($object).'::'.$property;

if (isset($this->writePropertyCache[$key])) {
$access = $this->writePropertyCache[$key];
} elseif (!$this->cache || !$access = $this->cache->fetch('write'.$key)) {
$access = array();
$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);

if (is_array($value) || $value instanceof \Traversable) {
$methods = $this->findAdderAndRemover($reflClass, $singulars);

if (null === $methods) {
// It is sufficient to include only the adders in the error
// 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) . '()", ';
} else {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_ADDER_AND_REMOVER;
$access[self::ACCESS_ADDER] = $methods[0];
$access[self::ACCESS_REMOVER] = $methods[1];
}
}

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

if ($reflClass->hasMethod($setter) && $reflClass->getMethod($setter)->isPublic()) {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_METHOD;
$access[self::ACCESS_NAME] = $setter;
} elseif ($reflClass->hasMethod('__set') && $reflClass->getMethod('__set')->isPublic()) {
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_PROPERTY;
$access[self::ACCESS_NAME] = $property;
} 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)) {
Copy link
Member

Choose a reason for hiding this comment

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

same issue here for the caching. It depends on the instance

// 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_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()", ' .
'"__set()" or "__call()" exist and have public access in class "%s".',
$property,
$guessedAdders,
$setter,
$reflClass->name
);
}
}

$this->writePropertyCache[$key] = $access;
if ($this->cache) {
$this->cache->save('write'.$key, $access);
}
}

// Any of the two methods is required, but not yet known
$singulars = null !== $singular ? array($singular) : (array) StringUtil::singularify($plural);
switch ($access[self::ACCESS_TYPE]) {
case self::ACCESS_TYPE_METHOD:
$object->{$access[self::ACCESS_NAME]}($value);
break;

if (is_array($value) || $value instanceof \Traversable) {
$methods = $this->findAdderAndRemover($reflClass, $singulars);
case self::ACCESS_TYPE_PROPERTY:
$object->{$access[self::ACCESS_NAME]} = $value;
break;

if (null !== $methods) {
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
Expand Down Expand Up @@ -329,51 +454,16 @@ private function writeProperty(&$object, $property, $singular, $value)
}

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

foreach ($itemsToAdd as $item) {
call_user_func(array($object, $methods[0]), $item);
call_user_func(array($object, $access[self::ACCESS_ADDER]), $item);
}
break;
Copy link
Member

Choose a reason for hiding this comment

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

wrong indentation


return;
} else {
// It is sufficient to include only the adders in the error
// 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).'()", ';
}
}

$setter = 'set'.$this->camelize($property);
$classHasProperty = $reflClass->hasProperty($property);

if ($reflClass->hasMethod($setter) && $reflClass->getMethod($setter)->isPublic()) {
$object->$setter($value);
} elseif ($reflClass->hasMethod('__set') && $reflClass->getMethod('__set')->isPublic()) {
$object->$property = $value;
} elseif ($classHasProperty && $reflClass->getProperty($property)->isPublic()) {
$object->$property = $value;
} 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.
$object->$property = $value;
} elseif ($this->magicCall && $reflClass->hasMethod('__call') && $reflClass->getMethod('__call')->isPublic()) {
// we call the getter and hope the __call do the job
$object->$setter($value);
} else {
throw new NoSuchPropertyException(sprintf(
'Neither the property "%s" nor one of the methods %s"%s()", '.
'"__set()" or "__call()" exist and have public access in class "%s".',
$property,
$guessedAdders,
$setter,
$reflClass->name
));
default:
throw new NoSuchPropertyException($access[self::ACCESS_NAME]);
}
}

Expand Down Expand Up @@ -445,4 +535,23 @@ private function isAccessible(\ReflectionClass $class, $methodName, $parameters)

return false;
}

/**
* @param PropertyPathInterface|string $propertyPath
*
* @return PropertyPath
*/
private function getPropertyPath($propertyPath)
{
if ($propertyPath instanceof PropertyPathInterface) {
return $propertyPath;
}

if (isset($this->propertyPathCache[$propertyPath])) {
return $this->propertyPathCache[$propertyPath];
}

$this->propertyPathCache[$propertyPath] = new PropertyPath($propertyPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

return $this->propertyPathCache[$propertyPath] = new PropertyPath($propertyPath);

return $this->propertyPathCache[$propertyPath];
}
}
2 changes: 1 addition & 1 deletion src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ public function testGetValueThrowsExceptionIfNotObjectOrArray($objectOrArray, $p

public function testGetValueWhenArrayValueIsNull()
{
$this->propertyAccessor = new PropertyAccessor(false, true);
$this->propertyAccessor = new PropertyAccessor(false);
$this->assertNull($this->propertyAccessor->getValue(array('index' => array('nullable' => null)), '[index][nullable]'));
}

Expand Down
6 changes: 6 additions & 0 deletions src/Symfony/Component/PropertyAccess/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@
"require": {
"php": ">=5.3.3"
},
"require-dev": {
"doctrine/cache": "~2.4"
},
"suggest": {
"doctrine/cache": "To cache access methods"
},
"autoload": {
"psr-0": { "Symfony\\Component\\PropertyAccess\\": "" }
},
Expand Down
0