Movatterモバイル変換

bendavies reviewed

src/Symfony/Component/Serializer/Serializer.php Outdated


		$cacheable =true;
		foreach ($this->normalizersas$normalizer) {
		if ($cacheable) {

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

bendaviesApr 25, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

but we are only interested inNormalizerInterface normalizers here? some only implementDenormalizerInterface likeArrayDenormalizer? i'm probably wrong...

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ok got it, sorry.

Copy link

Contributor

sroze commentedApr 25, 2018•
edited
Loading

this will dramatically improve the performance of the Serializer component

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

Copy link

Contributor

bendavies commentedApr 25, 2018•
edited
Loading

For me this only gave a 9% speed up, asgetNormalizer will evaluate every normalizer for scalar/nulls and then returnnull, which is the majority of mygetNormalizer calls.

joelwurtz reviewed

src/Symfony/Component/Serializer/Serializer.php Outdated

		privatefunctiongetNormalizer($data,$format,array$context)
		{
		if (\is_object($data) &&isset($this->normalizerCache[\get_class($data)][$format])) {
		return$this->normalizerCache[\get_class($data)][$format];

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

dunglas commentedApr 25, 2018•
edited
Loading

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

dunglas removed Bug Deprecation labels

Copy link

MemberAuthor

dunglas commentedApr 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'sbenchmark. 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

Copy link

MemberAuthor

dunglas commentedApr 25, 2018•
edited
Loading

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 reviewed

src/Symfony/Component/Serializer/Serializer.php Outdated

		*/
		privatefunctiongetNormalizer($data,$format,array$context)
		{
		$type =\is_object($data) ?\get_class($data) :\gettype($data);

Copy link

Contributor

jvasseurApr 25, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This could lead to buggy behavior sincegettype 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).

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ok i'll add a prefix for classes.

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

done

javiereguiluz reviewed

Apr 26, 2018

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

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What would you think about renamingNormalizerWithCacheableSupportsMethod toCacheableNormalizer?

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

dunglasApr 26, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

CacheableNormalizer is misleading (the result of the normalization isn't cacheable itself, it's only the call to thesupports 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 asupports* 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.

Copy link

Member

nicolas-grekasApr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

MissingInterface suffix.
What aboutCacheableSupportsInterface
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 reviewed

Apr 26, 2018

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

		*/
		publicfunctionvaryBy():array
		{
		returnarray('type');

Copy link

Contributor

OskarStarkApr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

srozeApr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

dunglasApr 26, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

(s/short/long/)

Copy link

Contributor

teohhanhuiApr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

nicolas-grekasApr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

stofApr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

nicolas-grekasApr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Indeed!

nicolas-grekas reviewed

Apr 26, 2018

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

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

Copy link

Member

nicolas-grekasApr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

src/Symfony/Component/Serializer/Serializer.php Outdated

		*/
		privatefunctiongetNormalizer($data,$format,array$context)
		{
		$type =\is_object($data) ?'c-'.\get_class($data) :\gettype($data);

Copy link

Member

nicolas-grekasApr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Contributor

bendaviesApr 26, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

src/Symfony/Component/Serializer/Serializer.php Outdated

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

Copy link

Member

nicolas-grekasApr 26, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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;

Copy link

Member

nicolas-grekas commentedApr 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? egContextFreeNormalizerInterface?
Once a normalizer implements this, it could be cached by type+format?
For more safety and in order to enforce the contract, I'd passnull 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?

Copy link

Contributor

jvasseur commentedApr 26, 2018

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

Copy link

Member

nicolas-grekas commentedApr 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.

Copy link

Member

stof commentedApr 27, 2018

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

Copy link

MemberAuthor

dunglas commentedApr 27, 2018•
edited
Loading

New implementation proposal:

Interface renamed toNormalizerWithCacheableSupportResultInterface (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: thesupports method depends of the context). I would keep it for now.

Copy link

MemberAuthor

dunglas commentedApr 27, 2018•
edited
Loading

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.

[Serializer] Cache the normalizer to use when possible

c1e850f

nicolas-grekas force-pushed theserializer-varyby branch frombbb4d4e toe89c236Compare

April 27, 2018 12:29

Copy link

Member

nicolas-grekas commentedApr 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 added this to the4.1 milestone

nicolas-grekas force-pushed theserializer-varyby branch 2 times, most recently from074b9b7 to11ecc0aCompare

April 27, 2018 12:47

carsonbot removed the Status: Needs Review label

nicolas-grekas approved these changes

carsonbot added the Status: Reviewed label

[Serializer] Generalize caching support a bit

16f8a13

nicolas-grekas force-pushed theserializer-varyby branch from11ecc0a to16f8a13Compare

April 27, 2018 13:09

Copy link

MemberAuthor

dunglas commentedApr 27, 2018

Nice improvements@nicolas-grekas 👍

Copy link

MemberAuthor

dunglas commentedApr 27, 2018

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

Copy link

Contributor

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

Yes. Will later today.

sroze reviewed

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

Copy link

Contributor

srozeApr 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

nicolas-grekasApr 27, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

bendavies commentedApr 27, 2018•
edited
Loading

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

98% improvement ongetNormalizer:

Copy link

Member

nicolas-grekas commentedApr 29, 2018

Thank you@dunglas.

nicolas-grekas merged commit16f8a13 intosymfony:master

Apr 29, 2018

nicolas-grekas added a commit that referenced this pull request

Apr 29, 2018

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

5b6df6f

… (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     <!-- seehttps://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        | todoStill 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@bendaviesCommits-------16f8a13 [Serializer] Generalize caching support a bitc1e850f [Serializer] Cache the normalizer to use when possible

Copy link

Contributor

emodric commentedApr 30, 2018•
edited
Loading

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

nicolas-grekas mentioned this pull request

Apr 30, 2018

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

Merged

Copy link

Member

nicolas-grekas commentedApr 30, 2018

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

Copy link

MemberAuthor

dunglas commentedMay 1, 2018•
edited
Loading

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

dunglas deleted the serializer-varyby branch

May 1, 2018 02:48

dunglas added a commit that referenced this pull request

May 3, 2018

feature#27105[Serializer] Add ->hasCacheableSupportsMethod() to Cac…

50fd769

…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 mentioned this pull request

May 7, 2018

Release v4.1.0-BETA1#27181

Merged

bendavies reviewed

Jul 9, 2018