Skip to content

[PropertyInfo] PhpDocExtractor should cache on class level #32073

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.

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
alexanderschnitzler opened this issue Jun 17, 2019 · 4 comments
Closed

[PropertyInfo] PhpDocExtractor should cache on class level #32073

alexanderschnitzler opened this issue Jun 17, 2019 · 4 comments

Comments

@alexanderschnitzler
Copy link

Symfony version(s) affected: 4.3.0

Description
When calling \TYPO3\CMS\Extbase\Reflection\PropertyInfo\Extractor\PhpDocExtractor::getTypes with a class and a property name, the result of the combination of arguments is cached – which is fantastic – but when repeatedly calling getTypes with the same class name but different property names (which is a common task), the type context is being generated from scratch for each property of the same class. Unfortunately, \phpDocumentor\Reflection\Types\ContextFactory::createForNamespace is quite CPU intense. Not caching the generated context can and most likely will lead to a massive performance issues.

Fetching the type information more than once per class and property is unlikely however. Therefore I expect very few cache hits in \Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor::getDocBlock.

How to reproduce

$propertyInfoExtractor = new PropertyInfoExtractor(
    [],
    [new PhpDocExtractor()]
);

$propertyInfoExtractor->getTypes('Foo\Bar\Baz\Class', 'foo');
$propertyInfoExtractor->getTypes('Foo\Bar\Baz\Class', 'bar');
$propertyInfoExtractor->getTypes('Foo\Bar\Baz\Class', 'baz');
...

The more property types of properties of the same class are evaluated, the more redundant type context objects are created.

Possible Solution

As mentioned before, the type contexts could be created and cached once per class.

@Nek-
Copy link
Contributor

Nek- commented Jun 17, 2019

Hello,

Thanks for taking the time to send feedback to the Symfony team. That's great.

I definitely agree with the fact that this needs to be cached. But in fact, Symfony already comes with a cache system. You can decorate your PropertyInfoExtractor. Here is your example, but cached:

$propertyInfoExtractor = new PropertyInfoCacheExtractor(new PropertyInfoExtractor(
    [],
    [new PhpDocExtractor()]
), $psrCacheInterfaceImplementation);
// So you can cache in files, but also redis or memcached or even doctrine

I may have misunderstood you and maybe this does not cover your needs, feel free to tell us or close the issue.

@alexanderschnitzler
Copy link
Author

alexanderschnitzler commented Jun 17, 2019

Well, I know that caching solution but it doesn't help with the issue as the caches are generated for each class-property-pair but the type context objects are still created again and again while processing multiple properties of the same class.

The context however, holds information regarding the class, not the property, so creating the context object once for all properties of the same class saves a huge amount of cpu cycles.

Let me show you some numbers.
Here is a comparison of two requests, processing the same amount of classes and properties. 1st request doesn't cache, the second does: https://blackfire.io/profiles/compare/ee456fc6-59ad-451e-afd9-e5d595144986/graph?settings[dimension]=wt&settings[display]=focused&settings[tabPane]=nodes&selected=phpDocumentor\Reflection\Types\ContextFactory%3A%3AcreateForNamespace&callname=phpDocumentor\Reflection\Types\ContextFactory%3A%3AcreateForNamespace

Here is the first request:
https://blackfire.io/profiles/adc673b9-af65-4a08-8eef-f468cf11eeb4/graph

Here is the second (with a cache):
https://blackfire.io/profiles/3785d24c-baa5-4690-b429-ac8dc14c876e/graph

You can see that createForNamespace is called 115 times because there are 16 classes with a total of 115 properties.

In the second request, createForNamespace is only called once per class, i.e. 16 times in total.

As you can see in the comparison, this avoids the execution of around 760.000 calls of ArrayIterator::current, resulting in a drop of 788ms, from 853ms to just 65ms.

Besides that I might note that the caching solution you mention is a second level cache, which is great btw, but the caching mechanism I was talking about is a 1st level cache like the one implemented here: https://github.com/symfony/property-info/blob/master/Extractor/PhpDocExtractor.php#L156

The only issue here, is that this kind of 1st level cache is quite useless unless gathering the type information more than once per class-property pair. And with a seconds level cache, this 1st level cache is completely useless. Important to me is the first hit and the redundant recreation of type contexts being recreated for each property of each class.

I opened this issue because since we use symfony/property-info in TYPO3, we have a huge performance issue. To fix this, I created a custom PhpDocExtractor, which can be seen here: https://review.typo3.org/c/Packages/TYPO3.CMS/+/61076

The custom extractor is stripped down to a bare minimum, but you can see the 1st level cache in line 86: https://review.typo3.org/c/Packages/TYPO3.CMS/+/61076/3/typo3/sysext/extbase/Classes/Reflection/PropertyInfo/Extractor/PhpDocExtractor.php#86

Thanks for the quick answer and feel free to ping me anytime if you need more feedback. :)

@javiereguiluz
Copy link
Member

Alexander, thanks a lot for creating this issue and providing so many detailed resources. The profile comparison is quite telling. Before trying to implement this feature, let's wait a bit and see if someone can come up with a potential problem for introducing this cache.

@bastnic
Copy link
Contributor

bastnic commented Jun 26, 2019

I've the same problem and it's even worse than your case @alexanderschnitzler, see attached PR for fix proposal.

@fabpot fabpot closed this as completed Jul 3, 2019
fabpot added a commit that referenced this issue Jul 3, 2019
This PR was merged into the 4.4 branch.

Discussion
----------

[PropertyInfo] add static cache to ContextFactory

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no (performance...)
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32073, api-platform/api-platform#1159
| License       | MIT
| Doc PR        | no

The issue is very very well described here #32073, and was also discussed a few weeks ago with @dunglas here api-platform/api-platform#1159.

`ContextFactory::createFromReflector` is heavy, and it's called redundanlty
by `Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor` for each property
and methods. Avoid that by parsing it only once and then use static cache

This is a quite big performance problem.

![Deepin Capture-écran_zone de sélection _20190626142041](https://user-images.githubusercontent.com/84887/60179692-8471c480-981e-11e9-9e3c-3f9c0b83b01b.png)

Commits
-------

063e880 [PropertyInfo] add static cache to ContextFactory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants