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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

[PropertyAccess] Major performance improvement #16294

wants to merge 8 commits into from

Conversation

dunglas
Copy link
Member
@dunglas dunglas commented Oct 20, 2015
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #16179
License MIT
Doc PR n/a

This PR improves performance of the PropertyAccess component of ~70%.

The two main changes are:

  • caching the PropertyPath initialization
  • caching the guessed access strategy

This is especially important for the ObjectNormalizer (Symfony Serializer) and the JSON-LD normalizer (API Platform) because they use the PropertyAccessor class in large loops (ex: normalization of a list of entities).

Here is the Blackfire comparison: https://blackfire.io/profiles/compare/c42fd275-2b0c-4ce5-8bf3-84762054d31e/graph

The code of the benchmark I've used (with Symfony 2.3 as dependency):

<?php

require 'vendor/autoload.php';

class Foo
{
    private $baz;
    public $bar;


    public function getBaz()
    {
        return $this->baz;
    }

    public function setBaz($baz)
    {
        $this->baz = $baz;
    }
}

use Symfony\Component\PropertyAccess\PropertyAccess;

$accessor = PropertyAccess::createPropertyAccessor();

$start = microtime(true);

for ($i = 0; $i < 10000; ++$i) {
    $foo = new Foo();
    $accessor->setValue($foo, 'bar', 'Lorem');
    $accessor->setValue($foo, 'baz', 'Ipsum');
    $accessor->getValue($foo, 'bar');
    $accessor->getValue($foo, 'baz');
}

echo 'Time: '.(microtime(true) - $start).PHP_EOL;

This PR also adds an optional support for Doctrine cache to keep access information across requests and improve the overall application performance (even outside of loops).

@dupuchba
Copy link
Contributor

👍

stof
stof reviewed Oct 20, 2015
View reviewed changes
src/Symfony/Component/PropertyAccess/PropertyAccessor.php

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

@stof
Copy link
Member
stof commented Oct 20, 2015

Using the object hash in the cache key can cause issues if your object is garbage collected and the same hash is reused (which is possible as it is based on the memory location of the object, which is available for reuse).

Btw, your benchmark is not relevant for the API platform use case: you are getting the values thousands of times on the same instance. Your API platform loop would be looping over objects, and so use separate instances. And most of your cache would not be usable in this case, because the object hash would be different and so the cache key would be different. Only the PropertyPath cache would enter into effect. I suggest you to update your benchmark.

What about caching only the logic relying on the class definition instead of caching the logic 8000 depending on the object ? This means that you would only need to check dynamic properties each time (and do it before calling the magic methods to respect the existing order of checks). But most cases would still benefit from the cache, and it would be shareable between all objects of the same class, making it much more likely to be reused (and making it shareable across requests too)

@dunglas
Copy link
Member Author
dunglas commented Oct 20, 2015

@stof I just changed the strategy (updated the PR) before you posted.

stof
stof reviewed Oct 20, 2015
View reviewed changes
src/Symfony/Component/PropertyAccess/PropertyAccessor.php

$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

@stof
Copy link
Member
stof commented Oct 20, 2015

@dunglas I suggest you to split this PR in 2 separate PRs:

  • one caching the PropertyPath instances, which can be ready quickly
  • another one caching the access resolution

You really have 2 independent optimizations here

stloyd
stloyd reviewed Oct 20, 2015
View reviewed changes
src/Symfony/Component/PropertyAccess/PropertyAccessor.php
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);

@aitboudad
Copy link
Contributor

IMO this should be considered as new feature ?

@dunglas
Copy link
Member Author
dunglas commented Oct 20, 2015

Benchmark and Blackfire profile updated.

@dunglas
Copy link
Member Author
dunglas commented Oct 20, 2015

@stof I'll move the PropertyPath caching in another PR but it's a bit annoying because I plan to use Doctrine cache for PropertyPath too.

@sescandell
Copy link
Contributor

Hi,

IMO, your benchmarks is not exhaustive. What about performance impact on "non-similar" objects?

If I remember well, form component is mainly based on PropertyPath one.
Assuming a very long form (I remember a PR about form performance improvement in that situation) with many fields, what would be the impact of these changes caching objects for "nothing"?

Your benchmark perfectly feet for scenarios you described (Serialization), but PropertyPath might be used somewhere else.

Maybe it has no impact (or improving it), just guessing.

@stof
Copy link
Member
stof commented Oct 20, 2015

@sescandell the impact on form would be increased memory usage (because of the cache being kept), but the only difference in the code is an array key lookup each time, which is O(1), and much faster than all the reflection-based logic.

And if we store the cache in a persistent cache to reuse it across requests, forms would also benefit from it, as a previous request might have warmed up the cache.

@stof
Copy link
Member
stof commented Oct 20, 2015

@dunglas your Doctrine cache integration is broken currently anyway: it cannot be merged as is as it would break BC due to the second argument being used already in newer branches.
And the Doctrine cache integration should be considered as a new feature and done in 2.8 IMO. Caching things internally in 2.3 is fine, because it is purely internal, but changing the signature in patch releases looks weird.

@stof
Copy link
Member
stof commented Oct 20, 2015

Btw, I'm not sure that the PropertyAccessor is the right place to put the PropertyPath caching. We have many places instantiating a PropertyPath outside it (for instance the Form and Validator components always create the instance externally and passes it to the PropertyAccessor).

We may want to introduce a PropertyPathFactory in 2.8, with several implementations:

  • a base implementation instantiating the PropertyPath
  • a caching implementation wrapping another implementation and caching it in memory
  • a doctrine cache implementation (in the bridge) using a persistent cache (this also assumes that we make the PropertyPath class serializable, as we will need to serialize it in the cache)

This is already what we do for the ChoiceListFactory in 2.7+ for instance.

Then, any place needing to create a PropertyPath would rely on the factory (making it an optional argument for BC, and creating a new CachingPropertyPathFactory(new PropertyPathFactory()) by default for instance). This would be BC because new PropertyPath() still works (but you won't get caching if you use it). This would allow to optimize other places using the PropertyPath too.

@stof
Copy link
Member
stof commented Oct 20, 2015

Btw, looking at the code of the PropertyPath constructor, I found a simpler optimization: the regex used in it can be optimized simply. I will send a PR for it. Then we will see whether your property path cache is still worth it or no.

sescandell
sescandell reviewed Oct 20, 2015
View reviewed changes
src/Symfony/Component/PropertyAccess/PropertyAccessor.php
$access[self::ACCESS_TYPE] = self::ACCESS_TYPE_METHOD;
$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)

stof
stof reviewed Oct 20, 2015
View reviewed changes
src/Symfony/Component/PropertyAccess/PropertyAccessor.php
$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;
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)

@dunglas
Copy link
Member Author
dunglas commented Oct 20, 2015

@stof OK with your proposal.

  • Removed Doctrine cache support from this PR (I'll open a new PR against 2.8 or master if it's too late for 2.8)
  • Removed PropertyPatch caching (I'll open another PR if the regex optim isn't enough)

stof
stof reviewed Oct 20, 2015
View reviewed changes
src/Symfony/Component/PropertyAccess/PropertyAccessor.php
default:
$reflClass = new \ReflectionClass($object);

if (!$reflClass->hasProperty($property) && 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.

the reflection part could be cached too, to avoid redoing it.

@dunglas
Copy link
Member Author
dunglas commented Oct 20, 2015

@stof problems you raised should be fixed now

@dunglas
Copy link
Member Author
dunglas commented Oct 20, 2015

AppVeyor errors not related.

sescandell
sescandell reviewed Oct 21, 2015
View reviewed changes
src/Symfony/Component/PropertyAccess/PropertyAccessor.php
$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.

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.

👍

@stof
Copy link
Member
stof commented Oct 21, 2015

@dunglas to make the code more readable, the cache building may be extracted to separate private methods

@dunglas
Copy link
Member Author
dunglas commented Oct 25, 2015

@stof done. Did you have the time to take a look at the PropertyPath regex?

@dunglas
Copy link
Member Author
dunglas commented Oct 29, 2015

ping @symfony/deciders

@fabpot
Copy link
Member
fabpot commented Oct 30, 2015

Thank you @dunglas.

@fabpot fabpot closed this Oct 30, 2015
fabpot added a commit that referenced this pull request Oct 30, 2015
@fabpot
bug #16294 [PropertyAccess] Major performance improvement (dunglas)
3b2d010 
This PR was squashed before being merged into the 2.3 branch (closes #16294).

Discussion
----------

[PropertyAccess] Major performance improvement

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #16179
| License       | MIT
| Doc PR        | n/a

This PR improves performance of the PropertyAccess component of ~70%.

The two main changes are:

* caching the `PropertyPath` initialization
* caching the guessed access strategy

This is especially important for the `ObjectNormalizer` (Symfony Serializer) and the JSON-LD normalizer ([API Platform](https://api-platform.com)) because they use the `PropertyAccessor` class in large loops (ex: normalization of a list of entities).

Here is the Blackfire comparison: https://blackfire.io/profiles/compare/c42fd275-2b0c-4ce5-8bf3-84762054d31e/graph

The code of the benchmark I've used (with Symfony 2.3 as dependency):

```php
<?php

require 'vendor/autoload.php';

class Foo
{
    private $baz;
    public $bar;

    public function getBaz()
    {
        return $this->baz;
    }

    public function setBaz($baz)
    {
        $this->baz = $baz;
    }
}

use Symfony\Component\PropertyAccess\PropertyAccess;

$accessor = PropertyAccess::createPropertyAccessor();

$start = microtime(true);

for ($i = 0; $i < 10000; ++$i) {
    $foo = new Foo();
    $accessor->setValue($foo, 'bar', 'Lorem');
    $accessor->setValue($foo, 'baz', 'Ipsum');
    $accessor->getValue($foo, 'bar');
    $accessor->getValue($foo, 'baz');
}

echo 'Time: '.(microtime(true) - $start).PHP_EOL;
```

This PR also adds an optional support for Doctrine cache to keep access information across requests and improve the overall application performance (even outside of loops).

Commits
-------

284dc75 [PropertyAccess] Major performance improvement
@sstok
Copy link
Contributor
sstok commented Oct 31, 2015

I really like Stofs idea for caching the determined strategy, and caching the property-path string to object 👍

@dunglas
Copy link
Member Author
dunglas commented Oct 31, 2015

I removed these features from this PR because they cannot be merged in 2.3 but I'll open PR new PRa against 2.8 yo add them back.

@fabpot
Copy link
Member
fabpot commented Nov 2, 2015

@dunglas I've just merged 2.3 into 2.7 without including your modifications as it conflicts badly with all the changes made since 2.3. Instead of trying to merge everything, I think it would be better to just create a new PR on 2.7. Can you take care of that? Thanks.

@dunglas
Copy link
Member Author
dunglas commented Nov 2, 2015

Ok I'll do it this week.

This was referenced Nov 4, 2015
Merged
Closed
fabpot added a commit that referenced this pull request Nov 5, 2015
@fabpot
bug #16462 [PropertyAccess] Fix dynamic property accessing. (dunglas)
4f7fd74 
This PR was merged into the 2.3 branch.

Discussion
----------

[PropertyAccess] Fix dynamic property accessing.

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Fix a bug regarding dynamic properties access introduced by #16294.

Commits
-------

916f9e0 [PropertyAccess] Test access to dynamic properties
352dfb9 [PropertyAccess] Fix dynamic property accessing.
fabpot added a commit that referenced this pull request Nov 5, 2015
@fabpot
bug #16463 [PropertyAccess] Port of the performance optimization from…
814c272 
… 2.3 (dunglas)

This PR was squashed before being merged into the 2.7 branch (closes #16463).

Discussion
----------

[PropertyAccess] Port of the performance optimization from 2.3

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #16179
| License       | MIT
| Doc PR        | n/a

Portage of #16294 in the 2.7 branch.

Commits
-------

aa4cc90 [PropertyAccess] Port of the performance optimization from 2.3
@dunglas dunglas mentioned this pull request Nov 14, 2015
Closed
This was referenced Nov 23, 2015
Merged
Merged
@webmozart
Copy link
Contributor

This is awesome, thanks @dunglas!!

fabpot added a commit that referenced this pull request Nov 28, 2015
@fabpot
minor #16547 [Serializer] Improve ObjectNormalizer performance (dunglas)
2659c8e 
This PR was squashed before being merged into the 2.8 branch (closes #16547).

Discussion
----------

[Serializer] Improve ObjectNormalizer performance

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #16179
| License       | MIT
| Doc PR        | n/a

Cache attributes detection in a similar way than in #16294 for PropertyAccess.

As for the PropertyAccess Component, I'll open another PR (in 2.8 or 3.1) allowing to cache these attributes between requests using Doctrine Cache.

@Tobion, can you try this PR with your benchmark to estimate the gain?

Commits
-------

683f0f7 [Serializer] Improve ObjectNormalizer performance
@dunglas dunglas mentioned this pull request Dec 4, 2015
Closed
nicolas-grekas added a commit that referenced this pull request Jun 8, 2016
@nicolas-grekas
feature #16838 [PropertyAccess] Add PSR-6 cache (dunglas)
ce28a86 
This PR was squashed before being merged into the 3.2-dev branch (closes #16838).

Discussion
----------

[PropertyAccess] Add PSR-6 cache

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Follow #16294

Commits
-------

4ccabcd [PropertyAccess] Add PSR-6 cache
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
0