[Serializer] Add a NormalizerDumper #22051

GuilhemN · 2017-03-18T11:00:30Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes, no new tests for now
Fixed tickets	#17516
License	MIT
Doc PR

This PR adds a functional NormalizerDumper to limit the cost of the ObjectNormalizer.
I decided to only support ->normalize() for now as it is the most used method and I think this is already a big step.

For this class:

class Foo
{
    /**
     * @Groups({"foo", "bar"})
     * @MaxDepth(2)
     */
    public $foo;
    public $bar;

    public function getFoo() {
        return $this->foo;
    }
}

It will generate:

use Symfony\Component\Serializer\Normalizer\NormalizerAwareInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerAwareTrait;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;
use Symfony\Component\Serializer\Normalizer\ObjectNormalizer;

/**
 * This class is generated.
 * Please do not update it manually.
 */
class FooNormalizer implements NormalizerInterface, NormalizerAwareInterface
{
    use NormalizerAwareTrait;

    public function normalize($object, $format = null, array $context = array())
    {

        $objectHash = spl_object_hash($object);
        if (isset($context[ObjectNormalizer::CIRCULAR_REFERENCE_LIMIT][$objectHash])) {
            throw new CircularReferenceException('A circular reference has been detected (configured limit: 1).');
        } else {
            $context[ObjectNormalizer::CIRCULAR_REFERENCE_LIMIT][$objectHash] = 1;
        }

        $groups = isset($context[ObjectNormalizer::GROUPS]) && is_array($context[ObjectNormalizer::GROUPS]) ? $context[ObjectNormalizer::GROUPS] : null;

        $output = array();
        if (isset($context[ObjectNormalizer::ENABLE_MAX_DEPTH])) {
            if (!isset($context['depth_Foo::foo'])) {
                $context['depth_Foo::foo'] = 1;
            } elseif (2 !== $context['depth_Foo::foo']) {
                ++$context['depth_Foo::foo'];
            }
        }

        if ((null === $groups || count(array_intersect($groups, array('foo', 'bar')))) && (!isset($context['attributes']) || isset($context['attributes']['foo']) || (is_array($context['attributes']) && in_array('foo', $context['attributes'], true))) && (!isset($context['depth_Foo::foo']) || 2 !== $context['depth_Foo::foo'])) {
            if (is_scalar($object->getFoo())) {
                $output['foo'] = $object->getFoo();
            } else {
                $subContext = $context;
                if (isset($context['attributes']['foo'])) {
                    $subContext['attributes'] = $context['attributes']['foo'];
                }

                $output['foo'] = $this->normalizer->normalize($object->getFoo(), $format, $context);
            }
        }
        if ((null === $groups) && (!isset($context['attributes']) || isset($context['attributes']['bar']) || (is_array($context['attributes']) && in_array('bar', $context['attributes'], true)))) {
            if (is_scalar($object->bar)) {
                $output['bar'] = $object->bar;
            } else {
                $subContext = $context;
                if (isset($context['attributes']['bar'])) {
                    $subContext['attributes'] = $context['attributes']['bar'];
                }

                $output['bar'] = $this->normalizer->normalize($object->bar, $format, $context);
            }
        }

        return $output;
    }

    public function supportsNormalization($data, $format = null, array $context = array())
    {
        return $data instanceof \Foo;
    }
}

I didn't write the tests yet, I'm waiting for feedbacks first.
You can try it using this.

GuilhemN · 2017-03-18T12:29:29Z

I ran some benchmarks with blackfire:

Comparing the generated normalizer provided in the header and the ObjectNormalizer (instanciated with no args, so there's no cache, it's not completely objective), I obtain https://blackfire.io/profiles/compare/055fe23b-e2ea-49bc-91be-80d50ce262cd/graph.
Comparing the generated normalizer and the GetSetMethodNormalizer, I have https://blackfire.io/profiles/compare/b65ebb08-1b17-4bc3-a0da-5de8fdb709f0/graph.

The GetSetMethodNormalizer should be faster than the ObjectNormalizer so we have at less a gain of 30%.

The benchmark of the generated normalizer is https://blackfire.io/profiles/c0303264-3e68-444c-8687-3b47a8a2205b/graph, according to it FooNormalizer::normalize() takes 22.2 µs.
The benchmark using the GetSetMethodNormalizer is https://blackfire.io/profiles/eb6c3ce8-b44b-4b09-9093-167b3b350638/graph, according to it GetSetMethodNormalizer::normalize() takes 127 µs.

soyuka · 2017-03-20T07:57:42Z

It's better without php-parser, it's not really useful here!

How would an Array of Foo's be normalized here? I assume it'd be a chain of ObjectNormalizer and FooNormalizer?

Is eval better (in term of performance) than require_once in your example?
On a real world example, how would the Normalizers be generated so that they can be easily reused?

I've done some hacking on my end, I'll do some bench! Nice job there!

GuilhemN · 2017-03-20T08:06:32Z

@soyuka the built-in serializer supports array.

Is eval better (in term of performance) than require_once in your example?

I think require_once is better to benefit from the optimisation and opcache, and eval is not available for some hostings.

On a real world example, how would the Normalizers be generated so that they can be easily reused?

We can decide that later but I would use a compiler pass that would register classes based on a glob pattern (defined in the config).

soyuka · 2017-03-20T09:24:12Z

I think require_once is better to benefit from the optimisation and opcache, and eval is not available for some hostings.

Then there's missing a php open tag <?php in the generated classes.

We can decide that later but I would use a compiler pass that would register classes based on a glob pattern (defined in the config).

Might be long to generate every normalizer, from what I understand there your normalizers are groups-independent, so that's a nice thing. Though, I tried to generate data with related entities and I only get the root entity. Take this gist for example, maybe I missed something!

src/Symfony/Component/Serializer/Dumper/NormalizerDumper.php

dunglas · 2017-03-20T16:05:19Z

Can't we move max depth / circular reference and other features shared with non-dumped normalizers in traits to ease the maintenance?

soyuka · 2017-03-20T16:05:22Z

I made some more benchmarks with big collections where each item has 4 relations to another item.

This is in dev mode, with a forced apcu cache. Times are from the profiler, can't run Blackfire on it because it's taking too long.

Without the normalizers the script is spending 35720ms in the serializer
With the cached normalizers, this time is reduced to 5897 ms

Another one is getting an 8000 collection of simple objects:

Without the normalizers we're spending 3112 ms in the serializer
With the cached normalizers 236 ms

We can see that this speed things a lot!

Here is a gist with the cache warmer and the compiler pass (made them in a hurry).

Edit:
One of our resource with many relations was spending ~300 ms in the serializer. Now it has been reduced to ~30ms.

Note that we could improve performances by avoiding the $this->normalizer->normalize calls. I'm not sure that we have the required metadata to do so for complex data, but we could avoid some calls for the simple data types (boolean, string etc.).

Also, it'd be nice to have a way to improve the generated normalizers (for example if we want metadata fields, for jsonld it could be @type and @id).

/Edit: For the metadata that's being used to generate the class, WDYT about using property-info instead of the classMetadataFactory? It'll get you more informations about the data Type which will lead to performances improvements (avoid the $this->normalizer->normalize call for simple data).

GuilhemN · 2017-03-20T17:54:20Z

Note that we could improve performances by avoiding the $this->normalizer->normalize calls. I'm not sure that we have the required metadata to do so for complex data, but we could avoid some calls for the simple data types (boolean, string etc.).

@soyuka that's planed but the PR is already pretty big and I preferred delaying it and having something simpler (certainly a bit slower) but still working.

Can't we move max depth / circular reference and other features shared with non-dumped normalizers in traits to ease the maintenance?

@dunglas what do you think we can share? The code is generated here so except by using eval it seems complicated to me to share code.

soyuka · 2017-03-20T19:40:24Z

@soyuka that's planed but the PR is already pretty big and I preferred delaying it and having something simpler (certainly a bit slower) but still working.

Isn't property-info a part of the serializer? I see no harm in already replacing the classMetadataFactory, or maybe that this class should in fact be using property-info would be easier 😄. Nevermind.

The code is generated here so except by using eval it seems complicated to me to share code.

+1 this is hard because the generated code is hard-coding attributes, which most of the code there relies on. One may think it'd be easier to maintain with a php-parser but I'm actually glad you removed it, it's overkill for such thing.

src/Symfony/Component/Serializer/Tests/Dumper/NormalizerDumperTest.php

src/Symfony/Component/Serializer/Dumper/NormalizerDumper.php

soyuka · 2017-03-21T10:50:14Z

I think we can do a Trait like this one to remove most of the logic from the actual normalizers:

Trait example

```php use Symfony\Component\Serializer\Exception\CircularReferenceException;
use Symfony\Component\Serializer\Normalizer\ObjectNormalizer;

trait NormalizerTrait
{
public function checkCircularReference($object, &$context)
{
$objectHash = spl_object_hash($object);
if (isset($context[ObjectNormalizer::CIRCULAR_REFERENCE_LIMIT][$objectHash])) {
throw new CircularReferenceException('A circular reference has been detected (configured limit: 1).');
} else {
$context[ObjectNormalizer::CIRCULAR_REFERENCE_LIMIT][$objectHash] = 1;
}
}

public function isAllowedAttribute($property, &$context) {
    $groups = isset($context[ObjectNormalizer::GROUPS]) && is_array($context[ObjectNormalizer::GROUPS]) ? $context[ObjectNormalizer::GROUPS] : null;

    if ((null === $groups) && (!isset($context['attributes']) || isset($context['attributes'][$property]) || (is_array($context['attributes']) && in_array($property, $context['attributes'], true)))) {
        return true;
    }

    return false;
}

public function getValue($value, $property, $format, &$context) {
    if (is_scalar($value)) {
        return [$property => $value];
    } else {
        return [$property => $this->normalizer->normalize($value, $format, $context));
    }
}

}

@dunglas is this what you had in mind?

https://github.com/GuilhemN/symfony/pull/2
</details>

dunglas · 2017-03-21T10:54:08Z

@soyuka indeed

8000

soyuka · 2017-03-21T11:24:52Z

src/Symfony/Component/Serializer/Dumper/NormalizerDumper.php

+    {
+        $reflectionClass = new \ReflectionClass($class);
+        if (!isset($context['class'])) {
+            $context['class'] = $reflectionClass->getShortName().'Normalizer';


I really think we should introduce a ClassNameConverter here, I made one on my fork that's just doing str_replace('\\', '_', $reflectionClass->getName()) to avoid collisions.

I think the namespace option is enough (maybe we should use it by default though).

Hmm, yes in fact I saw this option while working with it but I didn't try it. I'll, and yes I think we should make it default.

Rethinking about it, I think we should make the class option mandatory: in any case, the code using the NormalizerDumper will also have to generate the normalizer name to be able to call it.
I also don't expect the dumper to automatically put my normalizers in the same vendor than the model.

Yes that's correct. If it then works within a CompilerPass, it would need this same class name to create a Definition.

GuilhemN · 2017-03-21T18:11:03Z

I think we can do a Trait like this one to remove most of the logic from the actual normalizers:

I don't really see what we gain except having one more thing to maintain. I don't think it fits everyone's usecase, it is very generic while when you write a custom normalizer, you want something very specific imo.

src/Symfony/Component/Serializer/Dumper/NormalizerDumper.php

javiereguiluz · 2017-03-24T16:51:01Z

I don't have experiene with the Serializer component, so maybe there are strong reasons to merge this feature. But, for an outsider like me, this looks wrong for two reasons:

If this is done because the Serializer is slow, then we should focus on making it faster ... and not add a PHP code generator to create some optimized runtime code.
If this is done because the Serializer is verbose, then we should focus on making it less verbose ... and not add a PHP code generator to create the verbose code faster.

soyuka · 2017-03-24T16:59:33Z

If this is done because the Serializer is slow, then we should focus on making it faster ... and not add a PHP code generator to create some optimized runtime code.

IMHO this is exactly what's being done here, making the serializer faster (because yes it's slow).
The serializer is really well optimized but the PropertyAccessor will always be slower than calling getters directly. In fact, there is nothing faster than using generated accessors to serialize an object.
What bother's you is that this is generating code?

What are you meaning by verbose?

xabbuh · 2018-02-23T08:41:20Z

Normalizers are generated at runtime, the first time the user tries to normalize an object.

Does this mean that one cannot use this feature when using a cache that should be read-only? Would it be possible to generate normalizers on warmup (maybe as an opt-in feature)?

GuilhemN · 2018-02-23T13:18:47Z

Does this mean that one cannot use this feature when using a cache that should be read-only?

Indeed

Would it be possible to generate normalizers on warmup (maybe as an opt-in feature)?

Is it worth it ? I don't think that's really common and it would be easier to implement on the user's side (to implement it in the core we need an efficient way to filter the classes accepted, it would require at least a patterns and an exclude fields imo).

At least I think we should first wait to have a few feedbacks and if that's really a recurrent request only then implement it.

soyuka · 2018-02-23T15:29:51Z

Is it worth it ?

Exactly. It's feasible to do it in a custom cache warmer and it let you control how to choose the class to be normalized.
I must add that when you change your groups, you need to dump normalizers again.

GuilhemN · 2018-02-26T20:38:11Z

Exactly. It's feasible to do it in a custom cache warmer and it let you control how to choose the class to be normalized.

Lacking time to do this right now, I'd rather leave it to later as the PR is mergeable as is. Else this will unfortunately delay even more this PR.

I must add that when you change your groups, you need to dump normalizers again.

No need, serialization groups are supported by generated normalizers.

GuilhemN · 2018-11-03T17:07:06Z

PR rebased on master. Is it a no go merging this without a cache warmer?

fbourigault · 2018-11-05T15:26:01Z

Do you have some up-to-date benchmark since you rebased? I'm curious to see how much it may improve the ObjectNormalizer perfs compared to #28926.

fbourigault · 2018-11-06T08:29:43Z

Although this looks interesting if we only look at the benchmark figures, it looks hard to maintain and does not fit well in what @dunglas exposed in #19374 (comment).

What about following the Router path and leverage the PHP7 static array cache (see #28865 and #25909 by @nicolas-grekas).

We could leverage the PHP7 static array cache by decorating the PropertyAccessorInterface which is already used by ObjectNormalizer and also the PropertyListExtractorInterface when #28775 will be accepted and merged.

Also, I wrote a RFC to improve the performances of the ObjectNormalizer property access part (see #28926).

As the ObjectNormalizer is becoming more SOLID, I think it's better to improve performances by small steps instead of hiding real performance problems behind dumped code which will improve the performance of the Serializer but not the ObjectNormalizer. Those small improvements will also benefit to anyone using the PropertyAccess and PropertyInfo components either as standalone components or with the FrameworkBundle.

nicolas-grekas · 2019-04-03T14:44:04Z

Is this still relevant? There has been a lot of progress on the topic and we might prefer another approach now? Honestly I don't know, I'm just asking :)

GuilhemN · 2019-04-03T21:14:57Z

You're right, new proposals emerged lately and things seem to be accelerating on their side.

Maintaining this is indeed complicated compared to other solutions. I don't think we'll reach the same performance but if the difference is neglictible and other aspects are in favor of the other solutions it's ok :)

Let's close this and give other proposals a shot! (this weekend eventually)

carsonbot added Status: Needs Review Serializer Feature labels Mar 18, 2017

GuilhemN force-pushed the SERIALIZERAST branch 3 times, most recently from 258cd70 to 40eabea Compare March 18, 2017 11:28

GuilhemN changed the title ~~[Serializer] Add a NormalizerGenerator~~ [Serializer] Add a NormalizerDumper Mar 18, 2017

nicolas-grekas added this to the 3.x milestone Mar 18, 2017

teohhanhui suggested changes Mar 20, 2017

View reviewed changes

soyuka reviewed Mar 20, 2017

View reviewed changes

src/Symfony/Component/Serializer/Dumper/NormalizerDumper.php Outdated Show resolved Hide resolved

soyuka reviewed Mar 20, 2017

View reviewed changes

src/Symfony/Component/Serializer/Dumper/NormalizerDumper.php Outdated Show resolved Hide resolved

soyuka reviewed Mar 20, 2017

View reviewed changes

src/Symfony/Component/Serializer/Tests/Dumper/NormalizerDumperTest.php Show resolved Hide resolved

soyuka reviewed Mar 21, 2017

View reviewed changes

src/Symfony/Component/Serializer/Dumper/NormalizerDumper.php Show resolved Hide resolved

soyuka reviewed Mar 21, 2017

View reviewed changes

GuilhemN force-pushed the SERIALIZERAST branch from 30737c0 to e2a8f22 Compare March 22, 2017 14:13

GuilhemN mentioned this pull request Mar 22, 2017

[PropertyAccess] Allow custom methods on property accesses #18016

Closed

4 tasks

soyuka reviewed Mar 23, 2017

View reviewed changes

src/Symfony/Component/Serializer/Dumper/NormalizerDumper.php Outdated Show resolved Hide resolved

GuilhemN force-pushed the SERIALIZERAST branch 2 times, most recently from c3801fd to fd4b628 Compare February 19, 2018 21:12

[Serializer] Add a NormalizerGenerator

5fe5737

GuilhemN force-pushed the SERIALIZERAST branch from fd4b628 to c5cabc4 Compare February 19, 2018 21:57

Add a config option to enable the feature

32abf6c

GuilhemN force-pushed the SERIALIZERAST branch from c5cabc4 to 32abf6c Compare February 19, 2018 22:09

Merge branch 'master' into SERIALIZERAST

6c4f6be

nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018

GuilhemN mentioned this pull request Aug 27, 2018

[Serializer] CacheClassMetadataFactory performance #28272

Closed

soyuka mentioned this pull request Sep 17, 2018

Extremely slow DIC rebuild with api-platform enabled api-platform/api-platform#838

Closed

Merge branch 'master' into SERIALIZERAST

47adb72

GuilhemN force-pushed the SERIALIZERAST branch from def62e9 to bd45c0a Compare November 3, 2018 16:50

Update the tests

133766f

GuilhemN force-pushed the SERIALIZERAST branch from bd45c0a to 133766f Compare November 3, 2018 17:01

bendavies mentioned this pull request Nov 5, 2018

[RFC] Extract PropertyAccessor readProperty and writeProperty methods to allow faster ObjectNormalizer #28926

Closed

joelwurtz mentioned this pull request Feb 14, 2019

[AutoMapper] New component to automatically map a source object to a target object #30248

Closed

GuilhemN closed this Apr 3, 2019

nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

soyuka mentioned this pull request Dec 13, 2023

[Serializer] Add support for auto generated custom normalizers #52905

Open

Uh oh!

[Serializer] Add a NormalizerDumper #22051

[Serializer] Add a NormalizerDumper #22051

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!