Can anyone see why 5.5 fails?

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

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

reverted test change

Can this implement the kernelResettable interface to avoid memory leaks?

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

I checked outegeloen/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/aftercomposer ins tocomposer up:
https://blackfire.io/profiles/compare/557592f7-bab5-4a87-898b-1409069877fd/graph

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

THEN, once this is resolved, I noticed thativory-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.

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

@nicolas-grekas

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

head of 3.4 or 4.x?

Master

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

So which commit caused the speed slowdown?

@bendavies What's the status of this PR?

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

ok, thanks for the heads-up.

@sroze@dunglas to the rescue maybe?

i'm taking a look

Symfony\Component\Serializer\Mapping\ClassDiscriminatorFromClassMetadata (added in symfony4) is not cached, but calls toSymfony\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, decoratingMappingClassDiscriminatorFromClassMetadata?
2. add local caches toCacheClassMetadataFactory (this PR)

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.

@dunglas good spot

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

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

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 ofgetAttributeValue which can be improved according to#28457 (comment).

IMHO, we should work on reducing theClassMetadataFactoryInterface::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

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

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

indeed#28889 fixes the root cause.