[Serializer] Add a NormalizerDumper #22051
Conversation
258cd70 to
40eabea
Compare
|
I ran some benchmarks with blackfire:
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 |
|
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 I've done some hacking on my end, I'll do some bench! Nice job there! |
|
@soyuka the built-in serializer supports array.
I think require_once is better to benefit from the optimisation and opcache, and eval is not available for some hostings.
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). |
Then there's missing a php open tag
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! |
|
Can't we move max depth / circular reference and other features shared with non-dumped normalizers in traits to ease the maintenance? |
|
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.
Another one is getting an 8000 collection of simple objects:
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: Note that we could improve performances by avoiding the 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 /Edit: For the metadata that's being used to generate the class, WDYT about using |
@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.
@dunglas what do you think we can share? The code is generated here so except by using |
Isn't
+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. |
|
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 } |
|
@soyuka indeed |
| { | ||
| $reflectionClass = new \ReflectionClass($class); | ||
| if (!isset($context['class'])) { | ||
| $context['class'] = $reflectionClass->getShortName().'Normalizer'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think the namespace option is enough (maybe we should use it by default though).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes that's correct. If it then works within a CompilerPass, it would need this same class name to create a Definition.
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. |
|
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:
|
IMHO this is exactly what's being done here, making the serializer faster (because yes it's slow). What are you meaning by verbose? |
c3801fd to
fd4b628
Compare
fd4b628 to
c5cabc4
Compare
c5cabc4 to
32abf6c
Compare
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)? |
Indeed
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 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. |
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.
No need, serialization groups are supported by generated normalizers. |
def62e9 to
bd45c0a
Compare
bd45c0a to
133766f
Compare
|
PR rebased on master. Is it a no go merging this without a cache warmer? |
|
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. |
|
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 We could leverage the PHP7 static array cache by decorating the Also, I wrote a RFC to improve the performances of the ObjectNormalizer property access part (see #28926). As the |
|
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 :) |
|
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) |
This PR adds a functional
NormalizerDumperto limit the cost of theObjectNormalizer.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:
It will generate:
I didn't write the tests yet, I'm waiting for feedbacks first.
You can try it using this.