8000 [performance] provide a simpler name converter than MetadataAwareNameConverter · Issue #35085 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

[performance] provide a simpler name converter than MetadataAwareNameConverter #35085

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.

Sign up for GitHub

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

Jump to bottom
Closed
bastnic opened this issue Dec 22, 2019 · 6 comments
Closed

[performance] provide a simpler name converter than MetadataAwareNameConverter #35085

bastnic opened this issue Dec 22, 2019 · 6 comments
Labels
Performance Serializer Stalled

Comments

@bastnic
Copy link
Contributor
bastnic commented Dec 22, 2019

Description
I dug a lot it Symfony internals lastly and the name converter stack was very costly. I reduced it a lot with #35041... Then I realized than I don't need at all a name converter.

The default one is MetadataAwareNameConverter, that allow a fallbackNameConverter. But the fallback is called ONLY after having grab all the metadata, checked for a SerializerName. I never saw a project using this attribute, and it troubles me that it costs so much. So just giving a simpler NameConverter as fallback is not enough because the worse already happened.

I tried another way: replacing MetadataAwareNameConverter completely. It seems to work flawlessly and gives me a 11% performance improvement (AFTER the previously mentioned patch, so more than a 25% gain from master)

image (1)

<?php
namespace Lib\Core\Serializer\NameConverter;

use Symfony\Component\Serializer\NameConverter\NameConverterInterface;

/**
 * Do nothing name converter
 */
class NullNameConverter implements NameConverterInterface
{
    public function normalize($propertyName)
    {
        return $propertyName;
    }
    public function denormalize($propertyName)
    {
        return $propertyName;
    }
}
services:
    serializer.name_converter.metadata_aware: '@Lib\Core\Serializer\NameConverter\NullNameConverter'

Questions:

  • Did I miss something that I could break with that?
  • Should a simpler Name Converter should be added in Symfony?
  • Should it be the default? Or as least purposed by default
  • Maybe add a pass somewhere "hi you are not using any SerializerName and you didn't provide a NameConverter fallback, switching to NullNameConverter"?
@bastnic
Copy link
Contributor Author
bastnic commented Dec 22, 2019

ping #34708

@bastnic bastnic changed the title [performance] provide a simpler name converter thant MetadataAwareNameConverter [performance] provide a simpler name converter than MetadataAwareNameConverter Dec 22, 2019
@fbourigault
Copy link
Contributor

IIRC, with a compiler pass, you can use null as name converter!

bastnic added a commit to bastnic/symfony that referenced this issue Jan 7, 2020
@bastnic
[Serializer] Fix cache in MetadataAwareNameConverter
ded0739 
`isset` is used to test existence of values that is
`null` by default, which result to always bypass the cache
and force to do the calcul all the time.

This is a critical perf improvement in prod mode for an api.

Ref symfony#35085
bastnic added a commit to bastnic/symfony that referenced this issue Jan 7, 2020
@bastnic
[Serializer] Fix cache in MetadataAwareNameConverter
76db25e 
`isset` is used to test existence of values that is
`null` by default, which result to always bypass the cache
and force to do the calcul all the time.

This is a critical perf improvement in prod mode for an api.

Ref symfony#35085
@bastnic bastnic mentioned this issue Jan 7, 2020
Merged
bastnic added a commit to bastnic/symfony that referenced this issue Jan 7, 2020
@bastnic
[Serializer] Fix cache in MetadataAwareNameConverter
4862535 
`isset` is used to test existence of values that is
`null` by default, which result to always bypass the cache
and force to do the calculate all the time.

This is a critical perf improvement in prod mode for an api.

Ref symfony#35085
@bastnic
Copy link
Contributor Author
bastnic commented Jan 7, 2020

Hmm, cf #35252, there was a bug in the caching, so it will be less critical, but still useful ticket.

bastnic added a commit to bastnic/symfony that referenced this issue Jan 7, 2020
@bastnic
[Serializer] Fix cache in MetadataAwareNameConverter
dfd691f 
`isset` is used to test existence of values that is
`null` by default, which result to always bypass the cache
and force to do the calculate all the time.

This is a critical perf improvement in prod mode for an api.

Ref symfony#35085
bastnic added a commit to bastnic/symfony that referenced this issue Jan 7, 2020
@bastnic
[Serializer] Fix cache in MetadataAwareNameConverter
199fda6 
`isset` is used to test existence of values that is
`null` by default, which result to always bypass the cache
and force to do the calculate all the time.

This is a critical perf improvement in prod mode for an api.

Ref symfony#35085
bastnic added a commit to bastnic/symfony that referenced this issue Jan 7, 2020
@bastnic
[Serializer] Fix cache in MetadataAwareNameConverter
6449f92 
`isset` is used to test existence of values that is
`null` by default, which result to always bypass the cache
and force to do the calculate all the time.

This is a critical perf improvement in prod mode for an api.

Ref symfony#35085
fabpot added a commit that referenced this issue Jan 8, 2020
@fabpot
bug #35252 [Serializer] Fix cache in MetadataAwareNameConverter (bast…
9589dee 
…nic)

This PR was merged into the 4.4 branch.

Discussion
----------

[Serializer] Fix cache in MetadataAwareNameConverter

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | ref #35085
| License       | MIT
| Doc PR        |

`isset` is used to test existence of values that is `null` by default, which result to always bypass the cache and force to do the calculation all the time.

This is a serious perf improvement in prod mode for an api.

![image](https://user-images.githubusercontent.com/84887/71933779-38c3ae80-31a3-11ea-8871-8e57cec87a89.png)

![image](https://user-images.githubusercontent.com/84887/71933675-074ae300-31a3-11ea-8e84-7adad3e6c96f.png)

Commits
-------

6449f92 [Serializer] Fix cache in MetadataAwareNameConverter
@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@carsonbot
Copy link

Hello? This issue is about to be closed if nobody replies.

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

hultberg pushed a commit to hultberg/symfony that referenced this issue Sep 17, 2021
@bastnic
[Serializer] Fix cache in MetadataAwareNameConverter
76ecc21 
`isset` is used to test existence of values that is
`null` by default, which result to always bypass the cache
and force to do the calculate all the time.

This is a critical perf improvement in prod mode for an api.

Ref symfony#35085
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Performance Serializer Stalled
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@bastnic @fbourigault @xabbuh @carsonbot
0