doctrine / DoctrineBundle

Symfony Bundle for Doctrine ORM and DBAL
https://www.doctrine-project.org/projects/doctrine-bundle.html
MIT License
4.72k stars 453 forks source link

Undo deprecation of `metadata_cache_driver` configuration #1393

Closed ruudk closed 3 years ago

ruudk commented 3 years ago

After upgrading from 2.2 to 2.3 we noticed that the metadata_cache_driver option was deprecated. We removed the configuration key to get rid of the deprecation warning. We didn't think about it but after a while we noticed that our application started to slow down significantly. After investigation we noticed it was related to this metadata cache.

We are running a very large (9 years old) Symfony 5.3 application with almost 10.000 PHP files. The metadata caching allowed us to reduce our load times. We don't make changes to your entities that often as compared to other code changes. By having the metadata cache enabled we shave 100ms off our request load times.

I created a PR to bring back some of the functionality that we need but it was rejected:

Following up on @ostrolucky comment here:

We may consider undeprecating the option if there is enough interest, but since you are the only one speaking out, that's not enough. Since you have such mature project and complex workflow involving file watchers, you may as well write a compiler pass adjusting metadata cache. My experience speak against having persistent metadata cache in dev environment, it was constant struggle with changes not having any effect and having to nuke whole cache manually. Create an issue if interested and let's see if we get more feedback.

So this issue is to see if more people are using the metadata caching driver and want to keep using it.

I'd also like to know more about the reasoning and urge to deprecate the metadata caching configuration. Is there an issue/PR that explains the reasoning?

dmaicher commented 3 years ago

I'd also like to know more about the reasoning and urge to deprecate the metadata caching configuration. Is there an issue/PR that explains the reasoning?

There is multiple PRs about this topic. Here the original PR that introduced the new caching: https://github.com/doctrine/DoctrineBundle/pull/1196#issuecomment-724655625

So the idea was that independent of the metadata_cache_driver configuration we would use the fastest cache available when in no-debug mode which would be the new PhpArrayAdapter cache.

Later on while providing compatibility with PSR-6 caches @alcaeus changed the approach slightly here. So now we actually only register the fast PhpArrayAdapter if there is no other cache configured and we are in no-debug mode.

From my point of view there are not many use-cases where one would like to have a persistent cache while being in debug mode.

@ruudk your use-case could be handled with a custom compiler pass, right? Just wire the cache services yourself.

I don't have a strong opinion on this topic though. I would also be fine with reverting the deprecation and keeping the config to have full flexibility.

ostrolucky commented 3 years ago

Ok. If someone wants to do the work for undeprecating it and create a PR, I'm ok with merging it. I think that in the end, we should try to avoid deprecating options that are not deprecated in ORM/DBAL.

ruudk commented 3 years ago

@ostrolucky Thanks, I appreciate it. I will create the initial PR. But I might need some guidance in doing it right.

ruudk commented 3 years ago

@ostrolucky Created a draft PR to undo the deprecation. Could you approve the tests so that we can see how it works?

ruudk commented 3 years ago

Thanks for merging #1405.

I ran 2 Blackfire profiles (with 10 samples):

A: No metadata cache driver configured, and thus the smart cache warmer is enabled in debug mode B: Metadata cache driver configured that caches on file system.

I get a 20% performance improvement (on every request) with version B 🎉

Screenshot 2021-10-08 at 10 10 18@2x