8000 [Serializer] add a local cache to CacheClassMetadataFactory by bendavies · Pull Request #28457 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Serializer] add a local cache to CacheClassMetadataFactory #28457

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
wants to merge 1 commit into from
Closed

[Serializer] add a local cache to CacheClassMetadataFactory #28457

wants to merge 1 commit into from

Conversation

bendavies
Copy link
Contributor
@bendavies bendavies commented Sep 13, 2018 •
edited by nicolas-grekas
Loading
Q A
Branch? 3.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #28272
License MIT
Doc PR

ping @dunglas

This PR improves the performance of the Serializer in certain benchmarks,
https://www.goetas.com/blog/whats-new-in-jmsserializer-v20/ (https://github.com/goetas/ivory-serializer-benchmark) to be specific.

FYI, egeloen/ivory-serializer-benchmark#11 was also applied to obtain the results below.

Before PR:

bin/benchmark --vertical-complexity 60  --horizontal-complexity 60  --iteration 3
Ivory\Tests\Serializer\Benchmark\IvoryBenchmark | 0.89570792516073
Ivory\Tests\Serializer\Benchmark\SymfonyObjectNormalizerBenchmark | 3.3825397491455
Ivory\Tests\Serializer\Benchmark\SymfonyGetSetNormalizerBenchmark | 0.879989306132
Ivory\Tests\Serializer\Benchmark\JmsBenchmark | 0.68816534678141
Ivory\Tests\Serializer\Benchmark\JmsMinimalBenchmark | 0.49477465947469

After PR:

bin/benchmark --vertical-complexity 60  --horizontal-complexity 60  --iteration 3
Ivory\Tests\Serializer\Benchmark\IvoryBenchmark | 0.90302999814351
Ivory\Tests\Serializer\Benchmark\SymfonyObjectNormalizerBenchmark | 0.92719697952271
Ivory\Tests\Serializer\Benchmark\SymfonyGetSetNormalizerBenchmark | 0.42902104059855
Ivory\Tests\Serializer\Benchmark\JmsBenchmark | 0.66095995903015
Ivory\Tests\Serializer\Benchmark\JmsMinimalBenchmark | 0.48206241925557

The improvement of the Symfony Object and GetSet normalizer benchmarks is significant.

Testing the change was a bit annoying, as you can see. Any better ideas?

@bendavies
Copy link
Contributor Author

Can anyone see why 5.5 fails?

@ostrolucky
Copy link
Contributor

Seems bad idea to decorate psr caches with local caches. Memory leaks, lost control over caches in user space.

@dunglas
Copy link
Member
dunglas commented Sep 13, 2018 •
edited
Loading

I'm not sure it is worth it to test that the local cache is actually used. Such tests are very hard to maintain.

@bendavies
Copy link
Contributor Author
bendavies commented Sep 13, 2018 via email

@bendavies
Copy link
Contributor Author

reverted test change

@goetas
Copy link
Contributor
goetas commented Sep 14, 2018 •
edited
Loading

Can this implement the kernel Resettable interface to avoid memory leaks?

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Sep 17, 2018 •
edited
Loading

Hello, thanks for the PR. I'm not sure this is the right fix. Here is why:

I checked out egeloen/ivory-serializer-benchmark#11 and did a straight composer install: the perf number was not that different from others for SymfonyObjectNormalizerBenchmark.
Then I did a composer update, checking out the latest master. THEN that number started to diverge.

Here is a comparison profile before/after composer ins to composer up:
https://blackfire.io/profiles/compare/557592f7-bab5-4a87-898b-1409069877fd/graph

As you can see, the number of calls to getMetadataFor increased by +544454 times! That's the first thing we should look at.

THEN, once this is resolved, I noticed that ivory-serializer-benchmark is using ApcuAdapter. WHY? Would it use ArrayAdapter with its "serialize" option set to false, the numbers would be better for Symfony. Also this is not the cache adapter that is used in Symfony full-stack by default (it's PhpArrayAdapter).

IF ArrayAdapter is suited (ie sharing the same instance is fine), then we could/should chain an ArrayAdapter on top instead of inlining this logic here. To be confirmed by numbers.

PS:

Can this implement the kernel Resettable interface to avoid memory leaks?

not needed as class info is static, and the number of files is finite, so this wouldn't qualify as a memory leak to me.

@bendavies
Copy link
Contributor Author

Hey @nicolas-grekas thanks for the detailed review! I'll take another look later!

@nicolas-grekas nicolas-grekas 8000 added this to the 3.4 milestone Sep 17, 2018
@bendavies
Copy link
Contributor Author
bendavies commented Sep 17, 2018 •
edited
Loading

@nicolas-grekas

Then I did a composer update, checking out the latest master

head of 3.4 or 4.x?

@nicolas-grekas
Copy link
Member

Master

@bendavies
Copy link
Contributor Author
bendavies commented Sep 17, 2018 •
edited
Loading

duh thanks. have reproduced that performance decrease between 3.4 and master.

@nicolas-grekas nicolas-grekas mentioned this pull request Sep 23, 2018
Closed
@goetas
Copy link
Contributor
goetas commented Sep 23, 2018

So which commit caused the speed slowdown?

@bendavies
Copy link
Contributor Author
bendavies commented Sep 23, 2018 via email

@fabpot
Copy link
Member
fabpot commented Oct 10, 2018

@bendavies What's the status of this PR?

@bendavies
Copy link
Contributor Author

@fabian performance decrease in symfony 4 needs investigating. i haven't had time yet, but plan to.

@fabpot
Copy link
Member
fabpot commented Oct 10, 2018

ok, thanks for the heads-up.

@nicolas-grekas
Copy link
Member

@sroze @dunglas to the rescue maybe?

@bendavies
Copy link
Contributor Author

i'm taking a look

@bendavies
Copy link
Contributor Author
bendavies commented 8000 Oct 10, 2018 •
edited
Loading

Symfony\Component\Serializer\Mapping\ClassDiscriminatorFromClassMetadata (added in symfony4) is not cached, but calls to Symfony\Component\Serializer\Mapping\Factory\CacheClassMetadataFactory::getMetadataFor which is cached, but results in many cache lookups, which are slow.

https://blackfire.io/profiles/compare/890441d8-bfb6-430b-a56f-a96fe137cd9f/graph

So the local cache in this PR fixes the issue, reducing the cache lookups.

Optimally, we could:

  1. add a caching (CacheItemPoolInterface + local) ClassDiscriminatorResolverInterface, decorating MappingClassDiscriminatorFromClassMetadata?
  2. add local caches to CacheClassMetadataFactory (this PR)

@dunglas
Copy link
Member
dunglas commented Oct 10, 2018 •
edited
Loading

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Serializer/Normalizer/ObjectNormalizer.php#L114 looks faulty too. This method is called for every attributes, but could be called only one time per object. WDYT @sroze.

@bendavies
Copy link
Contributor Author

@dunglas good spot

@bendavies
Copy link
Contributor Author

@nicolas-grekas @dunglas feedback on my last comment?
I'll fix the issue dunglas found.

@nicolas-grekas
Copy link
Member

I'd be happy to have a new bench once the patch is applied to see if the perf issue remains after :)

@dunglas dunglas mentioned this pull request Oct 16, 2018
Closed
@fbourigault
Copy link
Contributor
fbourigault commented Oct 16, 2018 •
edited
Loading

I did some profiling. All where run using the following command: blackfire run bin/benchmark --iteration 10 --horizontal-complexity 4 --vertical-complexity 4.

First I did a profile to see which of arrayAdapter or apcuAdapter is the best:
https://blackfire.io/profiles/compare/6f937d24-47a9-4a47-912c-1541149beb83/graph
It looks like that arrayAdapter is worse. That's why further profiling are done using the apcuAdapter.

Symfony 3.4 vs this PR:
https://blackfire.io/profiles/compare/b24b6c9b-3652-4e59-9463-2ab035fc1c5a/graph
As we can see, this PR does not help at all to increase serializer speed.

master vs this PR rebased:
https://blackfire.io/profiles/compare/e9a70e1f-3594-45fd-9a4c-62919df8a85a/graph
This help a lot, but mostly on the path of getAttributeValue which can be improved according to #28457 (comment).

IMHO, we should work on reducing the ClassMetadataFactoryInterface::getMetadataFor calls to get better perfs.

EDIT : I forgot to disable serialization in arrayAdapter for my first comparison. Here are the new results: I forgot to disable serialization in the arrayAdapter. Here are the new comparison : https://blackfire.io/profiles/compare/d6b3637a-5c0b-4bdb-bcc2-db8a0b77667a/graph.
Using ArrayAdapter with serialization disabled looks equivalent to using apcuAdapter.

EDIT2 : I opened a PR based on those results: #28889

@bendavies
Copy link
Contributor Author

as an aside, ArrayAdapter with its "serialize" option set to false should be faster. ArrayAdapter(0, false)

@nicolas-grekas
Copy link
Member

I closed by mistake, but was going to ask if #28889 does solve this now?
I'll reopen if not, please advise :)

@bendavies
Copy link
Contributor Author

indeed #28889 fixes the root cause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunglas dunglas dunglas approved these changes

Assignees
No one assigned
Labels
Serializer Status: Needs Work
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
8 participants
@bendavies @ostrolucky @dunglas @goetas @nicolas-grekas @fabpot @fbourigault @carsonbot
0