Skip to content
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] Fix cache in MetadataAwareNameConverter #35252

Conversation

@bastnic
Copy link
Contributor

bastnic commented Jan 7, 2020

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

image

@bastnic bastnic requested a review from dunglas as a code owner Jan 7, 2020
@bastnic bastnic force-pushed the bastnic:feature/35085-fix-cache-MetadataAwareNameConverter branch from 76db25e to 4862535 Jan 7, 2020
@bastnic bastnic force-pushed the bastnic:feature/35085-fix-cache-MetadataAwareNameConverter branch 2 times, most recently from dfd691f to 199fda6 Jan 7, 2020
`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 #35085
@bastnic bastnic force-pushed the bastnic:feature/35085-fix-cache-MetadataAwareNameConverter branch from 199fda6 to 6449f92 Jan 7, 2020
@bastnic

This comment has been minimized.

Copy link
Contributor Author

bastnic commented Jan 7, 2020

FYI, it depends of the number of property in the response, so could be way more than 5% (I got 8 in a previous one).

@natebrunette

This comment has been minimized.

Copy link

natebrunette commented Jan 8, 2020

@bastnic Is this behavior documented somewhere? These results are impressive, I'm curious where you learned that array_key_exists is faster, as all the benchmarks I've seen show the opposite.

@OskarStark

This comment has been minimized.

Copy link
Contributor

OskarStark commented Jan 8, 2020

@natebrunette IIRC it’s not about the benchmark of these functions but about checking issue on null even if the key exists. Not the key is checked instead of the value which only forces a recalculate if really needed. The perf boost comes from correct usage not one function over the other. Am I right @bastnic?

@bastnic

This comment has been minimized.

Copy link
Contributor Author

bastnic commented Jan 8, 2020

@natebrunette IIRC it’s not about the benchmark of these functions but about checking issue on null even if the key exists. Not the key is checked instead of the value which only forces a recalculate if really needed. The perf boost comes from correct usage not one function over the other. Am I right @bastnic?

I couldn't say it better. Thanks @OskarStark.

@fabpot
fabpot approved these changes Jan 8, 2020
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 8, 2020

Thank you @bastnic.

fabpot added a commit that referenced this pull request Jan 8, 2020
…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
@fabpot fabpot merged commit 6449f92 into symfony:4.4 Jan 8, 2020
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Your code looks good.
Details
@bastnic bastnic deleted the bastnic:feature/35085-fix-cache-MetadataAwareNameConverter branch Jan 8, 2020
This was referenced Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.