[Serializer] Cache the normalizer to use when possible #27049

dunglas · 2018-04-25T15:27:46Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#24436
License	MIT
Doc PR	symfony/symfony-docs#10316

Still a WIP:

Support supportsDenormalization

As this will dramatically improve the performance of the Serializer component, can we consider introducing this change in 4.1 @symfony/deciders?

ping @bendavies

bendavies · 2018-04-25T15:59:24Z

src/Symfony/Component/Serializer/Serializer.php

        foreach ($this->normalizers as $normalizer) {
+            if ($cacheable) {


think this needs to be inside the $normalizer instanceof NormalizerInterface check

No, if a normalizer in the chain before the matching one doesn't implement this new interface, we cannot use the cache (or it will be a BC break).

but we are only interested in NormalizerInterface normalizers here? some only implement DenormalizerInterface like ArrayDenormalizer? i'm probably wrong...

Ok got it, sorry.

sroze · 2018-04-25T16:16:16Z

this will dramatically improve the performance of the Serializer component

Do you have some nice Blackfire profile to show us the difference? 😃

bendavies · 2018-04-25T16:31:43Z

For me this only gave a 9% speed up, as getNormalizer will evaluate every normalizer for scalar/nulls and then return null, which is the majority of my getNormalizer calls.

joelwurtz · 2018-04-25T17:02:56Z

src/Symfony/Component/Serializer/Serializer.php

@@ -202,8 +205,21 @@ public function supportsDenormalization($data, $type, $format = null, array $con
     */
    private function getNormalizer($data, $format, array $context)
    {
+        if (\is_object($data) && isset($this->normalizerCache[\get_class($data)][$format])) {
+            return $this->normalizerCache[\get_class($data)][$format];


There will be some BC break here, let's imagine two normalizers with same class and format where:

The first one (having a high priority) vary on data so not cacheable

The second one (having a lower priorirty) vary on class only, so is cacheable

When a data will get the second one, then it will have more priority than the first one (as it will be cached and returned here everytime).

No because we don't cache anything if one of the normalizer in the chain before the one that marches is not cacheable (even if the normalizer finally used is cacheable).

dunglas · 2018-04-25T17:13:48Z

@bendavies I added support for primitive types and excluded denormalizers. Can you try again?

dunglas · 2018-04-25T20:43:27Z

Here is a Blackfire comparison: https://blackfire.io/profiles/compare/ca1d8a07-335f-4a37-8ea7-369f3983d044...0563674b-fc0e-4ab4-9e6d-1b9a089651ad/graph?settings%5Bdimension%5D=wt&settings%5Bdisplay%5D=landscape&settings%5BtabPane%5D=nodes&selected=&callname=main()

Improvement of 9.27%.

I used an adapted version of @egeloen's benchmark. The gain will probably be bigger on real projects (especially API Platform ones) where a lot of normalizers are registered.

Here is the adaptation:

        $this->serializer = new Serializer(
            [
+                new DataUriNormalizer(),
+                new DateTimeNormalizer(),
+                new DateIntervalNormalizer(),
+                new ConstraintViolationListNormalizer(),
+                new JsonSerializableNormalizer(),
                new GetSetMethodNormalizer($classMetadataFactory)
            ],
            [new JsonEncoder()]
        );
    }

And the command used: blackfire run bin/benchmark -i100 -sSymfonyGetSetNormalizer --iteration 100 --vertical-complexity 4 --horizontal-complexity 4

dunglas · 2018-04-25T20:50:44Z

Here is a profile from of a real project: https://blackfire.io/profiles/compare/a5c802f8-5c45-4d06-ae6f-ed63b4f23b8a/graph (provided by @bendavies)

36% for the whole app!

jvasseur · 2018-04-25T20:55:20Z

src/Symfony/Component/Serializer/Serializer.php

@@ -202,11 +205,37 @@ public function supportsDenormalization($data, $type, $format = null, array $con
     */
    private function getNormalizer($data, $format, array $context)
    {
+        $type = \is_object($data) ? \get_class($data) : \gettype($data);


This could lead to buggy behavior since gettype can return strings that are valid class names (https://3v4l.org/bAJiO).

(this would be a really hard to get edge-case but still possible).

Ok i'll add a prefix for classes.

javiereguiluz · 2018-04-26T07:44:48Z

src/Symfony/Component/Serializer/Normalizer/NormalizerWithCacheableSupportsMethod.php

+ *
+ * @author Kévin Dunglas <dunglas@gmail.com>
+ */
+interface NormalizerWithCacheableSupportsMethod


What would you think about renaming NormalizerWithCacheableSupportsMethod to CacheableNormalizer?

It's the supports method that is cacheable not the normalizer itself if I'm not mistaking.

CacheableNormalizer is misleading (the result of the normalization isn't cacheable itself, it's only the call to the supports method).

But the performance improvement is so huge that I think caching should be the default behavior, and disabling the cache should be opt-out. Having a normalizer with a supports* method result that isn't cacheable is a edge case.

I would like to make the cache opt-out instead of opt-in, but it will be a BC break... On the other hand it improves DX and performance for everybody.

Missing Interface suffix.
What about CacheableSupportsInterface
I'd also add the supports method in the interface if possible.
Also, what about using constants for possible values?
VARY_BY_TYPE, etc.?

OskarStark · 2018-04-26T08:57:19Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

+     */
+    public function varyBy(): array
+    {
+        return array('type');


shouldn't we use short array notation here, as PHP 7.1 is the min version for 4.x?

Nop, this has been discussed numerous times already and we keep the sort array notation for consistency 🙃

(s/short/long/)

It'll be fixed in Symfony 5, right? 😂😂😂

It'll be reconsidered at last when 3.4 will be EOLed I suppose.

when 2.8 gets EOLed. 3.4 is not an issue. We can migrate it as it supports PHP 5.5+

nicolas-grekas · 2018-04-26T17:36:50Z

src/Symfony/Component/Serializer/Normalizer/NormalizerWithCacheableSupportsMethod.php

+ *
+ * @author Kévin Dunglas <dunglas@gmail.com>
+ */
+interface NormalizerWithCacheableSupportsMethod


Missing Interface suffix.
What about CacheableSupportsInterface
I'd also add the supports method in the interface if possible.
Also, what about using constants for possible values?
VARY_BY_TYPE, etc.?

nicolas-grekas · 2018-04-26T17:40:28Z

src/Symfony/Component/Serializer/Serializer.php

@@ -202,11 +205,37 @@ public function supportsDenormalization($data, $type, $format = null, array $con
     */
    private function getNormalizer($data, $format, array $context)
    {
+        $type = \is_object($data) ? 'c-'.\get_class($data) : \gettype($data);


does it make any sense to cache by internal type?
I feel like only classes should allow caching but I might be wrong
if the answer is yes, I'd suggest using the prefix on the gettype side, would be better for performance.

what it's doing is caching the fact that no normalizers supported a primitive, which allows a shortcut to escape testing again.

nicolas-grekas · 2018-04-26T17:42:09Z

src/Symfony/Component/Serializer/Serializer.php

+        $type = \is_object($data) ? 'c-'.\get_class($data) : \gettype($data);
+        if (
+            isset($this->normalizerCache[$type][$format]) ||
+            (isset($this->normalizerCache[$type]) && \array_key_exists($format, $this->normalizerCache[$type]))


if (isset($this->normalizerCache[$type][[$format])) looks enough to me here, provided the case for primitive types is done this way:

$this->normalizerCache[$type][$format] = false;

return $this->normalizerCache[$type][$format] ?: null;

nicolas-grekas · 2018-04-26T21:05:04Z

Some additional thought:
the current interface slows down the non-cacheable normalizers, because of the extra checks in the loop, esp. the array_diff check. But given the actually used vary-by is only "type" here, can't we specialize on this and use a labelling interface instead? eg ContextFreeNormalizerInterface?
Once a normalizer implements this, it could be cached by type+format?
For more safety and in order to enforce the contract, I'd pass null as $data when this interface is implemented (or maybe a string hinting the situation when dumped, for DX?), so that devs cannot mess up with $data once they opted-in.
Ideally also, this PR should allow reverting #24228, by moving from local caches to external caches. Is that possible?

jvasseur · 2018-04-26T23:58:39Z

@nicolas-grekas I like your idea but instead of the marker interface I would use interface with a supportNormalizationForType(string $type) method that is called instead of the supportNormalization so we don't have to pass something with a special meaning as $data

nicolas-grekas · 2018-04-27T04:41:21Z

supportNormalizationForType

*AndFormat - But not sure about that because the (de)normalizer interfaces ask about supportsNormalization. It'd be strange to require implementing two similar methods, especially when the new one would basically just call the current on with a dummy$data as there is no other possible implementation.

stof · 2018-04-27T07:34:42Z

The implementation does not seem to care about the varying though. It always varies the cache by both type and format.

dunglas · 2018-04-27T10:00:00Z

New implementation proposal:

Interface renamed to NormalizerWithCacheableSupportResultInterface (still not really fan of it...)
No method to implement
array_diff call removed

The gain is higher now, 16% instead of 9%: https://blackfire.io/profiles/compare/ca1d8a07-335f-4a37-8ea7-369f3983d044...b6a8383e-e4e9-4d9c-b514-2e31a47fbff1/graph?settings%5Bdimension%5D=wt&settings%5Bdisplay%5D=landscape&settings%5BtabPane%5D=nodes&selected=&callname=main()
Can you try it on your project @bendavies?

We should also update the documentation to encourage to always use this new interface when possible (= almost always).

@nicolas-grekas the local cache still has a benefit if (and only if) at least 1 normalizer in the chain doesn't implement this new interface (in some case, there is a real use case: the supports method depends of the context). I would keep it for now.

dunglas · 2018-04-27T10:18:42Z

For more safety and in order to enforce the contract, I'd pass null as $data when this interface is implemented (or maybe a string hinting the situation when dumped, for DX?), so that devs cannot mess up with $data once they opted-in.

It's not possible, most normalizers use $data at least to guess its type.

nicolas-grekas · 2018-04-27T12:31:56Z

@dunglas I just push-forced on your fork: the first commit is exactly yours (squashed from previous ones), the 2nd is mine: it adds caching to denormalizers and implements additional logic allowing to revert #24228.

dunglas · 2018-04-27T13:10:56Z

Nice improvements @nicolas-grekas 👍

dunglas · 2018-04-27T13:11:18Z

@bendavies can you try the last version before we merge it?

bendavies · 2018-04-27T13:13:06Z

Yes. Will later today.

sroze · 2018-04-27T13:27:48Z

src/Symfony/Component/Serializer/Serializer.php

+
+        foreach ($this->normalizerCache[$format][$type] as $k => $cached) {
+            $normalizer = $this->normalizers[$k];
+            if ($cached || $normalizer->supportsNormalization($data, $format, $context)) {


Just to clarify maybe you can replace $cached by $isCacheableAndSupportsNormalization?

bof, this is a local variable, and the concern is separated by 5 lines...

bendavies · 2018-04-27T14:55:31Z

Real world ApiPlatform app comparison (tldr: 36% improvement):
https://blackfire.io/profiles/compare/dc9c0964-fa3a-44dc-845c-b7ee4a2608f9/graph

98% improvement on getNormalizer:

nicolas-grekas · 2018-04-29T06:37:00Z

Thank you @dunglas.

@bendavies

… (dunglas, nicolas-grekas) This PR was merged into the 4.1-dev branch. Discussion ---------- [Serializer] Cache the normalizer to use when possible | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | #24436  | License | MIT | Doc PR | todo Still a WIP: * [x] Support `supportsDenormalization` As this will dramatically improve the performance of the Serializer component, can we consider introducing this change in 4.1 @symfony/deciders? ping @bendavies Commits ------- 16f8a13 [Serializer] Generalize caching support a bit c1e850f [Serializer] Cache the normalizer to use when possible

emodric · 2018-04-30T20:45:25Z

@dunglas @nicolas-grekas It seems that this breaks usage of my custom normalizers. They do not implement NormalizerWithCacheableSupportResultInterface because they do use custom logic to decide if they support normalizing provided data so now normalizing my objects always falls back to Symfony\Component\Serializer\Normalizer\ObjectNormalizer

nicolas-grekas · 2018-04-30T21:47:19Z

@emodric good catch. Can you please check #27105?

dunglas · 2018-05-01T02:47:41Z

@emodric can you provide a failing test case so we can iterate?

…heableSupportsMethodInterface (nicolas-grekas) This PR was merged into the 4.1-dev branch. Discussion ---------- [Serializer] Add ->hasCacheableSupportsMethod() to CacheableSupportsMethodInterface | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Enhances the interface introduced in #27049 to make it possible to dynamically define if "supports" methods are cacheable. Commits ------- 04b3692 [Serializer] Add ->hasCacheableSupportsMethod() to CacheableSupportsMethodInterface

bendavies · 2018-07-09T12:39:54Z

src/Symfony/Component/Serializer/Serializer.php

     */
    protected $normalizers = array();

+    private $cachedNormalizers;


@nicolas-grekas what is this property for? it it set below but never used again.

ah, $this->normalizers is protected so could be edited by extensions. in which case you'd want to reset the cache?

yes, that's the purpose, allowing efficient caching without breaking BC of the protected property (which is now internal so will be removed in 5.0

…dunglas, javiereguiluz) This PR was merged into the 4.1 branch. Discussion ---------- [Serializer] Cache the normalizer to use when possible symfony/symfony#27049 Commits ------- 3e2e30f Minor reword 8d6ca70 RST 8e09eeb RST ccdf894 [Serializer] Cache the normalizer to use when possible

dunglas requested a review from nicolas-grekas April 25, 2018 15:27

carsonbot added Status: Needs Review Serializer Bug Feature Deprecation labels Apr 25, 2018

bendavies reviewed Apr 25, 2018

View reviewed changes

joelwurtz reviewed Apr 25, 2018

View reviewed changes

dunglas removed Bug Deprecation labels Apr 25, 2018

jvasseur reviewed Apr 25, 2018

View reviewed changes

javiereguiluz reviewed Apr 26, 2018

View reviewed changes

OskarStark reviewed Apr 26, 2018

View reviewed changes

nicolas-grekas reviewed Apr 26, 2018

View reviewed changes

[Serializer] Cache the normalizer to use when possible

c1e850f

nicolas-grekas force-pushed the serializer-varyby branch from bbb4d4e to e89c236 Compare April 27, 2018 12:29

nicolas-grekas added this to the 4.1 milestone Apr 27, 2018

nicolas-grekas force-pushed the serializer-varyby branch 2 times, most recently from 074b9b7 to 11ecc0a Compare April 27, 2018 12:47

carsonbot removed the Status: Needs Review label Apr 27, 2018

nicolas-grekas approved these changes Apr 27, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Apr 27, 2018

[Serializer] Generalize caching support a bit

16f8a13

nicolas-grekas force-pushed the serializer-varyby branch from 11ecc0a to 16f8a13 Compare April 27, 2018 13:09

sroze reviewed Apr 27, 2018

View reviewed changes

nicolas-grekas merged commit 16f8a13 into symfony:master Apr 29, 2018

nicolas-grekas mentioned this pull request Apr 30, 2018

[Serializer] Add ->hasCacheableSupportsMethod() to CacheableSupportsMethodInterface #27105

Merged

dunglas deleted the serializer-varyby branch May 1, 2018 02:48

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

bendavies reviewed Jul 9, 2018

View reviewed changes

dunglas mentioned this pull request Sep 10, 2018

[Serializer] Cache the normalizer to use when possible symfony/symfony-docs#10316

Merged

bendavies mentioned this pull request Nov 30, 2018

Fetching normalizers not scalable / slow. #29387

Closed

bradjones1 mentioned this pull request Oct 16, 2023

[Serializer] Change visibility of Serializer::getNormalizer and ::getDenormalizer from private to protected #52084

Closed

		foreach ($this->normalizers as $normalizer) {
		if ($cacheable) {

[Serializer] Cache the normalizer to use when possible #27049

[Serializer] Cache the normalizer to use when possible #27049

Conversation

dunglas commented Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendavies Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze commented Apr 25, 2018 • edited Loading

bendavies commented Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Apr 25, 2018 • edited Loading

dunglas commented Apr 25, 2018

dunglas commented Apr 25, 2018 • edited Loading

jvasseur Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendavies Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Apr 26, 2018

jvasseur commented Apr 26, 2018

nicolas-grekas commented Apr 27, 2018

stof commented Apr 27, 2018

dunglas commented Apr 27, 2018 • edited Loading

dunglas commented Apr 27, 2018 • edited Loading

nicolas-grekas commented Apr 27, 2018

dunglas commented Apr 27, 2018

dunglas commented Apr 27, 2018

bendavies commented Apr 27, 2018 via email • edited by nicolas-grekas Loading

Choose a reason for hiding this comment

nicolas-grekas Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

bendavies commented Apr 27, 2018 • edited Loading

nicolas-grekas commented Apr 29, 2018

emodric commented Apr 30, 2018 • edited Loading

nicolas-grekas commented Apr 30, 2018

dunglas commented May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

dunglas commented Apr 25, 2018 •

edited

Loading

bendavies Apr 25, 2018 •

edited

Loading

sroze commented Apr 25, 2018 •

edited

Loading

bendavies commented Apr 25, 2018 •

edited

Loading

dunglas commented Apr 25, 2018 •

edited

Loading

dunglas commented Apr 25, 2018 •

edited

Loading

jvasseur Apr 25, 2018 •

edited

Loading

dunglas Apr 26, 2018 •

edited

Loading

dunglas Apr 26, 2018 •

edited

Loading

bendavies Apr 26, 2018 •

edited

Loading

nicolas-grekas Apr 26, 2018 •

edited

Loading

dunglas commented Apr 27, 2018 •

edited

Loading

dunglas commented Apr 27, 2018 •

edited

Loading

bendavies commented Apr 27, 2018 via email •

edited by nicolas-grekas

Loading

nicolas-grekas Apr 27, 2018 •

edited

Loading

bendavies commented Apr 27, 2018 •

edited

Loading

emodric commented Apr 30, 2018 •

edited

Loading

dunglas commented May 1, 2018 •

edited

Loading

nicolas-grekas Jul 9, 2018 •

edited

Loading