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

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

Merged
merged 2 commits into from Apr 29, 2018

Conversation

@dunglas
Member

dunglas commented Apr 25, 2018

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

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

This comment has been minimized.

@bendavies

bendavies Apr 25, 2018

Contributor

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

This comment has been minimized.

@dunglas

dunglas Apr 25, 2018

Member

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).

This comment has been minimized.

@bendavies

bendavies Apr 25, 2018

Contributor

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

This comment has been minimized.

@dunglas

dunglas Apr 25, 2018

Member

Ok got it, sorry.

@sroze

This comment has been minimized.

Member

sroze commented Apr 25, 2018

this will dramatically improve the performance of the Serializer component

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

@bendavies

This comment has been minimized.

Contributor

bendavies commented Apr 25, 2018

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.

@@ -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];

This comment has been minimized.

@joelwurtz

joelwurtz Apr 25, 2018

Contributor

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).

This comment has been minimized.

@dunglas

dunglas Apr 25, 2018

Member

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

This comment has been minimized.

Member

dunglas commented Apr 25, 2018

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

@dunglas

This comment has been minimized.

Member

dunglas commented Apr 25, 2018

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

This comment has been minimized.

Member

dunglas commented Apr 25, 2018

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!

@@ -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 comment has been minimized.

@jvasseur

jvasseur Apr 25, 2018

Contributor

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).

This comment has been minimized.

@dunglas

dunglas Apr 26, 2018

Member

Ok i'll add a prefix for classes.

This comment has been minimized.

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

This comment has been minimized.

@javiereguiluz

javiereguiluz Apr 26, 2018

Member

What would you think about renaming NormalizerWithCacheableSupportsMethod to CacheableNormalizer?

This comment has been minimized.

@soyuka

soyuka Apr 26, 2018

Contributor

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

This comment has been minimized.

@dunglas

dunglas Apr 26, 2018

Member

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.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 26, 2018

Member

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.?

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

This comment has been minimized.

@OskarStark

OskarStark Apr 26, 2018

Contributor

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

This comment has been minimized.

@sroze

sroze Apr 26, 2018

Member

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

This comment has been minimized.

@dunglas

dunglas Apr 26, 2018

Member

(s/short/long/)

This comment has been minimized.

@teohhanhui

teohhanhui Apr 26, 2018

Contributor

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 26, 2018

Member

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

This comment has been minimized.

@stof

stof Apr 26, 2018

Member

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

This comment has been minimized.

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 26, 2018

Member

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.?

@@ -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);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 26, 2018

Member

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.

This comment has been minimized.

@bendavies

bendavies Apr 26, 2018

Contributor

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

$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]))

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 26, 2018

Member

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

This comment has been minimized.

Member

nicolas-grekas commented Apr 26, 2018

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

This comment has been minimized.

Contributor

jvasseur commented Apr 26, 2018

@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

This comment has been minimized.

Member

nicolas-grekas commented Apr 27, 2018

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

This comment has been minimized.

Member

stof commented Apr 27, 2018

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

@dunglas

This comment has been minimized.

Member

dunglas commented Apr 27, 2018

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

This comment has been minimized.

Member

dunglas commented Apr 27, 2018

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

This comment has been minimized.

Member

nicolas-grekas commented Apr 27, 2018

@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.

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

@dunglas

This comment has been minimized.

Member

dunglas commented Apr 27, 2018

Nice improvements @nicolas-grekas 👍

@dunglas

This comment has been minimized.

Member

dunglas commented Apr 27, 2018

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

@bendavies

This comment has been minimized.

Contributor

bendavies commented Apr 27, 2018

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

This comment has been minimized.

@sroze

sroze Apr 27, 2018

Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 27, 2018

Member

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

@bendavies

This comment has been minimized.

Contributor

bendavies commented Apr 27, 2018

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

98% improvement on getNormalizer:
image

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 29, 2018

Thank you @dunglas.

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Apr 29, 2018

feature #27049 [Serializer] Cache the normalizer to use when possible…
… (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 <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #24436   <!-- #-prefixed issue number(s), if any -->
| 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

This comment has been minimized.

Contributor

emodric commented Apr 30, 2018

@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

This comment has been minimized.

Member

nicolas-grekas commented Apr 30, 2018

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

@dunglas

This comment has been minimized.

Member

dunglas commented May 1, 2018

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

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

dunglas added a commit that referenced this pull request May 3, 2018

feature #27105 [Serializer] Add ->hasCacheableSupportsMethod() to Cac…
…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

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

*/
protected $normalizers = array();
private $cachedNormalizers;

This comment has been minimized.

@bendavies

bendavies Jul 9, 2018

Contributor

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

This comment has been minimized.

@bendavies

bendavies Jul 9, 2018

Contributor

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 9, 2018

Member

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

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Sep 12, 2018

minor #10316 [Serializer] Cache the normalizer to use when possible (…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment