Bug #88033

Massive performance degration since symfony/property-info

Added by Christian Kuhn 4 months ago. Updated 17 days ago.

Status:
Resolved
Priority:
Must have
Assignee:
-
Category:
Performance
Target version:
-
Start date:
2019-03-29
Due date:
% Done:

100%

TYPO3 Version:
10
PHP Version:
Tags:
Complexity:
Is Regression:
Sprint Focus:

Description

Offending commit: bcde6c0177aba345b0bd363c4519892a132ef279 for https://review.typo3.org/#/c/Packages/TYPO3.CMS/+/59454/ https://forge.typo3.org/issues/87457

Core master has a massive performance issues since #87457, rendering uncached backend close to unusable, especially if xdebug is loaded at the same time.

This is obvious if calling backend after clearing caches, and if using the install tool that runs uncached all the time.

with xdebug:
  • install tool first request without patch: 318ms
  • install tool first request with patch: 1990ms (factor >6)
  • extension manager with empty caches, without patch: 2800ms
  • extension manager with empty caches, with patch: 21270ms (thats 20 seconds, factor >7 !!!)
without xdebug:
  • install tool first request without patch: 41ms
  • install tool first request with patch: 123ms (factor ~2.5)
  • extension manager with empty caches, without patch: 550ms
  • extension manager with empty caches, with patch: 1400ms (factor ~2.5)

Related issues

Related to TYPO3 Core - Feature #87457: Use symfony/property-info to gather doc block information Closed 2019-01-16

Associated revisions

Revision e4cc43a4 (diff)
Added by Alexander Schnitzler 17 days ago

[BUGFIX] Improve performance with symfony/property-info

With the introduction of symfony/property-info, the class
\Symfony\Component\PropertyInfo\Extractor\PhpDocExtractor
has been used to extract property types from the php doc.

Unfortunately said extractor class doesn't cache the so
called context object, which is created repeatedly for
each property of each class.

The context object is used to determine non FQCN's and
its creation comes at very high costs.

To fix this issue, a custom PhpDocExtractor class has
been created which acts just like the original one, but
which caches the context objects.

Releases: master
Fixes: #88033
Change-Id: I54bec5b8adadeb6bde107547cbd115fa8be97526
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/61076
Tested-by: TYPO3com <>
Tested-by: Benni Mack <>
Tested-by: Andreas Fernandez <>
Reviewed-by: Benni Mack <>
Reviewed-by: Andreas Fernandez <>

History

#1 Updated by Christian Kuhn 4 months ago

  • Related to Feature #87457: Use symfony/property-info to gather doc block information added

#2 Updated by Christian Kuhn 4 months ago

  • Description updated (diff)

#3 Updated by Riccardo De Contardi 4 months ago

  • Category set to Performance

#4 Updated by Alexander Schnitzler 4 months ago

A few things to mention here:

1) Yes, first hit is slower, especially since we reverted a patch (don't know which one exactly right now), since all class properties are analyzed again and not just a subset. This can be fixed with more work in the validation area.

2) There is a plan to create a cache warmup command. It's part of the DI-patch.

3) I can't take care right now and not until middle of may. Let's discuss this in a few weeks, please.

#5 Updated by Alexander Schnitzler 2 months ago

Just a few words on what happened here:

symfony/property-info had been introduced as it offers a quite slim approach to detect types of properties, even non-fully-qualified ones. It's a slim approach compared to other approaches I tried in the months before, namely nikic/php-parser and phpdocumentor/reflection-docblock.
symfony/property-info also uses the extractor of phpdocumentor/reflection-docblock, but spares a lot of cpu cycles through caching and other things. The details are not relevant at this point.

However, in the first approach, not all properties of classes had been analysed but only those that need the support for the detection of non-fully-qualified class names.
Those properties are:

- Injection properties
- Relation properties

Due to that circumstance we enabled the type extraction only for models and properties of classes that had an inject annotation. Unfortunately this broke the validation resolver which needs to know the type of properties for recursive validation. So, we changed the ClassSchema to analyse all properties of all classes.

To solve this I propose the following changes:

- Reduce the lookup of types in ClassSchema on the mentioned properties (injection, relation)
- Do a separate reflection of classes in the ValidatorResolver that determines the types of properties.

I would still like to find a proper solution that enables us to do a proper reflection on everything with a small footprint but I don't see this happening at this moment.
As validation doesn't take place that offen compared to other requests and as I think that validation may take a bit longer than other requests, I think doing a separate reflection in the ValidatorResolver is acceptable. This contradicts my approach of having all reflection in one place but it would be a sane thing to do imo.

#6 Updated by Gerrit Code Review 28 days ago

  • Status changed from New to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61076

#7 Updated by Gerrit Code Review 28 days ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61076

#8 Updated by Gerrit Code Review 28 days ago

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61076

#9 Updated by Gerrit Code Review 27 days ago

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61076

#10 Updated by Alexander Schnitzler 17 days ago

btw: I created an issue in symfony/symfony that describes that the behaviour we experience is a bug: https://github.com/symfony/symfony/issues/32073

The bug is being dealt with in this PR: https://github.com/symfony/symfony/pull/32188

#11 Updated by Anonymous 17 days ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

Also available in: Atom PDF