\Symfony\Component\Serializer\Serializer::getNormalizer() is slow [#3017291]

Problem/Motivation

During performance testing in JSON:API module it showed that a HUGE portion of the time was spent fetching normalizers. You can check the whole story here:

Upstream issue in symfony/serializer: https://github.com/symfony/symfony/issues/29387. The problem in the end is that the issue is present only in symfony/serializer version 3.4.x and it's already resolved somewhere in 4.x.

As core is bound to 3.4.x how complex would it be to move to symfony/serializer that has this HUGE performance improvement.

Marking it as critical because of this 2 reasons (details):

Over ~100ms or more savings with cold caches. In my use case the savings are way over that.
Gets measurably worse with lots of contrib modules or large data sets... - the issue IS data bound and the only way to limit the effects of it, is to introduce the particular cache layer that is laready present in 4.x.

Proposed resolution

I see some options that all should be discussed and decided on:

Upgrade to latest possible serializer package - best, maybe hard...
Have the package upgraded and implement a backwards compatible adapter for it, so existing code can be fixed easily.
Have a patch (and maintain it) and ship core with a patched dependency.
Any other ideas? ...

This is affecting everything that uses the serializer systems mainly REST, GraphQL and JSON:API.

Remaining tasks

Discussion, decision, Plan, Patch, etc...

User interface changes

TBD.

API changes

TBD.

Data model changes

TBD.

Release notes snippet

TBD.

Comment	File	Size	Author
#25	3017291-23.patch	1.18 KB	br0ken
#10	3017291-10-2.patch	3.68 KB	ndobromirov
#8	drupal-symfony-serializer-34-41-3017291.patch	17.13 KB	ndobromirov

Comments

Comment #1

30 November 2018 at 18:01

ndobromirov created an issue. See original summary.

Comment #2

ndobromirov commented 30 November 2018 at 18:05

Issue summary:

View changes

Comment #3

ndobromirov commented 30 November 2018 at 18:09

Comment #4

ndobromirov commented 30 November 2018 at 18:11

Issue summary:

View changes

Comment #5

wim leers

Ghent 🇧🇪🇪🇺

commented 30 November 2018 at 21:16

Title:	Symfony serializer performance issues.	» \Symfony\Component\Serializer\Serializer::getNormalizer() is slow
Priority:	Critical	» Major

Critical means things are on fire. Things aren't on fire, they merely running hot, and have been for years :)

Comment #6

wim leers

Ghent 🇧🇪🇪🇺

commented 30 November 2018 at 21:16

Also: thank you very much for creating this issue! 👍 🎉

Comment #7

ndobromirov commented 4 December 2018 at 13:46

Let's have the ball rolling...

How much will that break updating to D8 serializer dependency to 4.1 :) ?

Comment #8

ndobromirov commented 4 December 2018 at 15:15

Status:

Active

» Needs review

Status	File	Size
new	drupal-symfony-serializer-34-41-3017291.patch	17.13 KB

Here is an initial patch. First observations is the PHP 7.1+.

Comment #9

4 December 2018 at 15:12

Status:

Needs review

» Needs work

The last submitted patch, 8: drupal-symfony-serializer-34-41-3017291.patch, failed testing. View results

Comment #10

ndobromirov commented 4 December 2018 at 15:18

Status	File	Size
new	3017291-10-2.patch	3.68 KB

Strange. Rebased the patch...

Comment #11

ndobromirov commented 4 December 2018 at 15:19

Status:

Needs work

» Needs review

Comment #12

4 December 2018 at 16:12

Status:

Needs review

» Needs work

The last submitted patch, 10: 3017291-10-2.patch, failed testing. View results

Comment #13

ndobromirov commented 4 December 2018 at 16:54

Issue tags:

+8.8.x

So the only blocker in the test suite is against PHP version limits, as 8.7 will try to support PHP 5, as discussed in #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7.

As I see it, this is an 8.8.x material, as at that point PHP 5 will no longer be supported and hopefully we will be able to up this one.

Comment #14

ndobromirov commented 10 December 2018 at 14:06

Is there a way to have this sooner?

Comment #15

wim leers

Ghent 🇧🇪🇪🇺

commented 10 December 2018 at 15:22

Title:	\Symfony\Component\Serializer\Serializer::getNormalizer() is slow	» [upstream] \Symfony\Component\Serializer\Serializer::getNormalizer() is slow
Status:	Needs work	» Postponed (maintainer needs more info)

Is there a way to have this sooner?

You'd have to land a change in Symfony 3.x, in https://github.com/symfony/symfony.

Comment #16

wim leers

Ghent 🇧🇪🇪🇺

commented 10 December 2018 at 15:22

Status:

Postponed (maintainer needs more info)

» Postponed

Comment #17

ndobromirov commented 11 December 2018 at 10:33

As seen in the upstream issue - change in 3.4 will not happen.

Final question I've marked this for Drupal 8.8 but is this viable to happen there or we will be waiting for Drupal 9?

Comment #18

wim leers

Ghent 🇧🇪🇪🇺

commented 11 December 2018 at 11:36

D'oh, I'd not noticed that. Yep, there's nothing we can do here then right now.

Maybe we can do this in 8.8, but I wouldn't get my hopes up. Related: #2937984: [META] Symfony 4.0 compatibility + #2976394: Allow Symfony 4.4 to be installed in Drupal 8.

Comment #19

ndobromirov commented 19 December 2018 at 15:03

For anyone that might be interested in patching this on 3.4 D8 now, you can try this.

Comment #20

19 December 2018 at 15:03

Version:

8.7.x-dev

» 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Comment #21

jibran

he/him

Urdu

Toronto, Canada

commented 4 June 2019 at 14:58

I think this can be un-postponed given #2976394: Allow Symfony 4.4 to be installed in Drupal 8 is almost done. Or should we wait for #3020303: Consider using Symfony flex to allow switching between major Symfony versions?

Whatever we decide the first step should be to collect the benchmark data, IMO.

Comment #22

ndobromirov commented 4 June 2019 at 15:41

I think this should be blocked on #3020303: Consider using Symfony flex to allow switching between major Symfony versions...

Comment #23

4 June 2019 at 15:41

Version:

8.8.x-dev

» 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Comment #24

4 June 2019 at 15:41

Version:

8.9.x-dev

» 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Comment #25

br0ken

Ukrainian

commented 6 June 2020 at 12:05

Version:	9.1.x-dev	» 9.0.x-dev
Status:	Postponed	» Needs review
Issue tags:	-Needs upstream feature, -8.8.x

Status	File	Size
new	3017291-23.patch	1.18 KB

Let's start another round here. I'm working on a test to prove the benefit of a patch like this.

Comment #26

6 June 2020 at 12:05

Version:

9.0.x-dev

» 9.1.x-dev

Drupal 9.0.10 was released on December 3, 2020 and is the final full bugfix release for the Drupal 9.0.x series. Drupal 9.0.x will not receive any further development aside from security fixes. Sites should update to Drupal 9.1.0 to continue receiving regular bugfixes.

Drupal-9-only bug reports should be targeted for the 9.1.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.2.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #27

6 June 2020 at 12:05

Version:

9.1.x-dev

» 9.3.x-dev

Drupal 9.1.10 (June 4, 2021) and Drupal 9.2.10 (November 24, 2021) were the last bugfix releases of those minor version series. Drupal 9 bug reports should be targeted for the 9.3.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #28

wim leers

Ghent 🇧🇪🇪🇺

commented 30 December 2021 at 09:54

Title:	[upstream] \Symfony\Component\Serializer\Serializer::getNormalizer() is slow	» \Symfony\Component\Serializer\Serializer::getNormalizer() is slow
Status:	Needs review	» Needs work

#3252872: Use CacheableSupportsMethodInterface for performance improvement in normalizers landed and basically did what this proposes but only for jsonapi.module's normalizers.

I think we should do the same here now? 🤓

Removing the [upstream] prefix because it's been fixed upstream, and we're using the updated upstream 👍