Project

General

Profile

Actions

Bug #88033

closed

Massive performance degration since symfony/property-info

Added by Christian Kuhn over 5 years ago. Updated almost 5 years ago.

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

100%

Estimated time:
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 1 (0 open1 closed)

Related to TYPO3 Core - Feature #87457: Use symfony/property-info to gather doc block informationClosedAlexander Schnitzler2019-01-16

Actions
Actions #1

Updated by Christian Kuhn over 5 years ago

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

Updated by Christian Kuhn over 5 years ago

  • Description updated (diff)
Actions #3

Updated by Riccardo De Contardi over 5 years ago

  • Category set to Performance
Actions #4

Updated by Alexander Schnitzler over 5 years 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.

Actions #5

Updated by Alexander Schnitzler over 5 years 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.

Actions #6

Updated by Gerrit Code Review over 5 years 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

Actions #7

Updated by Gerrit Code Review over 5 years 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

Actions #8

Updated by Gerrit Code Review over 5 years 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

Actions #9

Updated by Gerrit Code Review over 5 years 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

Actions #10

Updated by Alexander Schnitzler over 5 years 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

Actions #11

Updated by Anonymous over 5 years ago

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

Updated by Benni Mack almost 5 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF